From d7f23f5c75225fad545639ae0f1dcc763bec2eae Mon Sep 17 00:00:00 2001 From: David Neto Date: Fri, 26 Mar 2021 12:13:18 +0000 Subject: [PATCH] spirv-reader: Fix mixed-signedness binary ops Fixed: tint:666 Change-Id: I77331081fc5acc128be81e7b85a253bf6ebab608 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46140 Auto-Submit: David Neto Kokoro: Kokoro Commit-Queue: Ben Clayton Reviewed-by: Ben Clayton --- src/reader/spirv/function.cc | 3 +- src/reader/spirv/function_arithmetic_test.cc | 401 ++++++++++++++++--- src/reader/spirv/function_bit_test.cc | 393 +++++++++++++++--- src/reader/spirv/function_logical_test.cc | 32 +- src/reader/spirv/parser_impl.cc | 44 ++ src/reader/spirv/parser_impl.h | 13 + 6 files changed, 789 insertions(+), 97 deletions(-) diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc index 09eaa5bee1..a7e3664625 100644 --- a/src/reader/spirv/function.cc +++ b/src/reader/spirv/function.cc @@ -3169,7 +3169,8 @@ TypedExpression FunctionEmitter::MaybeEmitCombinatorialValue( auto binary_op = ConvertBinaryOp(opcode); if (binary_op != ast::BinaryOp::kNone) { auto arg0 = MakeOperand(inst, 0); - auto arg1 = MakeOperand(inst, 1); + auto arg1 = parser_impl_.RectifySecondOperandSignedness( + inst, arg0.type, MakeOperand(inst, 1)); auto* binary_expr = create(Source{}, binary_op, arg0.expr, arg1.expr); TypedExpression result{ast_type, binary_expr}; diff --git a/src/reader/spirv/function_arithmetic_test.cc b/src/reader/spirv/function_arithmetic_test.cc index 9396ca3391..a13c79da38 100644 --- a/src/reader/spirv/function_arithmetic_test.cc +++ b/src/reader/spirv/function_arithmetic_test.cc @@ -104,6 +104,15 @@ std::string AstFor(std::string assembly) { } })"; } + if (assembly == "cast_uint_v2int_40_30") { + return R"(Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + })"; + } if (assembly == "v2float_50_60") { return R"(TypeConstructor[not set]{ __vec_2__f32 @@ -474,8 +483,51 @@ TEST_P(SpvBinaryArithTest, EmitExpression) { << GetParam().ast_type << "\n {\n Binary[not set]{" << "\n " << GetParam().ast_lhs << "\n " << GetParam().ast_op << "\n " << GetParam().ast_rhs; - EXPECT_THAT(ToString(p->builder(), fe.ast_body()), HasSubstr(ss.str())) + auto got = ToString(p->builder(), fe.ast_body()); + EXPECT_THAT(got, HasSubstr(ss.str())) << "got:\n" << got << assembly; +} + +// Use this when the result might have extra bitcasts on the outside. +struct BinaryDataGeneral { + const std::string res_type; + const std::string lhs; + const std::string op; + const std::string rhs; + const std::string expected; +}; +inline std::ostream& operator<<(std::ostream& out, BinaryDataGeneral data) { + out << "BinaryDataGeneral{" << data.res_type << "," << data.lhs << "," + << data.op << "," << data.rhs << "," << data.expected << "}"; + return out; +} + +using SpvBinaryArithGeneralTest = + SpvParserTestBase<::testing::TestWithParam>; + +TEST_P(SpvBinaryArithGeneralTest, EmitExpression) { + const auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + %entry = OpLabel + %1 = )" + GetParam().op + + " %" + GetParam().res_type + " %" + GetParam().lhs + + " %" + GetParam().rhs + R"( + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) + << p->error() << "\n" << assembly; + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + std::ostringstream ss; + ss << R"(VariableConst{ + x_1 + none + )" + << GetParam().expected; + auto got = ToString(p->builder(), fe.ast_body()); + EXPECT_THAT(got, HasSubstr(ss.str())) << "got:\n" << got << assembly; } INSTANTIATE_TEST_SUITE_P( @@ -490,14 +542,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpIAdd", "int_40", "__i32", "ScalarConstructor[not set]{30}", "add", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpIAdd", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "add", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpIAdd", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "add", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpIAdd", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "add", @@ -505,15 +549,108 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpIAdd", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "add", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_IAdd_MixedSignedness, + SpvBinaryArithGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpIAdd", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + add + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpIAdd", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + add + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpIAdd", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + add + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpIAdd", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + add + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpIAdd", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "add", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpIAdd", "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + add + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpIAdd", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "add", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpIAdd", "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + add + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); INSTANTIATE_TEST_SUITE_P( SpvParserTest_FAdd, @@ -540,14 +677,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpISub", "int_40", "__i32", "ScalarConstructor[not set]{30}", "subtract", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpISub", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "subtract", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpISub", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "subtract", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpISub", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "subtract", @@ -555,15 +684,108 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpISub", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "subtract", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_ISub_MixedSignedness, + SpvBinaryArithGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpISub", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + subtract + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpISub", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + subtract + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpISub", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + subtract + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpISub", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + subtract + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpISub", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "subtract", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpISub", "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + subtract + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpISub", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "subtract", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpISub", "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + subtract + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); INSTANTIATE_TEST_SUITE_P( SpvParserTest_FSub, @@ -590,14 +812,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpIMul", "int_40", "__i32", "ScalarConstructor[not set]{30}", "multiply", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpIMul", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "multiply", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpIMul", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "multiply", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpIMul", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "multiply", @@ -605,15 +819,108 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpIMul", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "multiply", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_IMul_MixedSignedness, + SpvBinaryArithGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpIMul", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + multiply + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpIMul", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + multiply + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpIMul", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + multiply + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpIMul", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + multiply + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpIMul", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "multiply", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpIMul", "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + multiply + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpIMul", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "multiply", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpIMul", "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + multiply + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); INSTANTIATE_TEST_SUITE_P( SpvParserTest_FMul, @@ -1203,7 +1510,7 @@ TEST_F(SpvBinaryArithTestBasic, OuterProduct) { // TODO(dneto): OpIAddCarry // TODO(dneto): OpISubBorrow -// TODO(dneto): OpIMulExtended +// TODO(dneto): OpUMulExtended // TODO(dneto): OpSMulExtended } // namespace diff --git a/src/reader/spirv/function_bit_test.cc b/src/reader/spirv/function_bit_test.cc index d1db30b27c..643b33863e 100644 --- a/src/reader/spirv/function_bit_test.cc +++ b/src/reader/spirv/function_bit_test.cc @@ -163,6 +163,49 @@ TEST_P(SpvBinaryBitTest, EmitExpression) { << assembly; } +// Use this when the result might have extra bitcasts on the outside. +struct BinaryDataGeneral { + const std::string res_type; + const std::string lhs; + const std::string op; + const std::string rhs; + const std::string expected; +}; +inline std::ostream& operator<<(std::ostream& out, BinaryDataGeneral data) { + out << "BinaryDataGeneral{" << data.res_type << "," << data.lhs << "," + << data.op << "," << data.rhs << "," << data.expected << "}"; + return out; +} + +using SpvBinaryBitGeneralTest = + SpvParserTestBase<::testing::TestWithParam>; + +TEST_P(SpvBinaryBitGeneralTest, EmitExpression) { + const auto assembly = CommonTypes() + R"( + %100 = OpFunction %void None %voidfn + %entry = OpLabel + %1 = )" + GetParam().op + + " %" + GetParam().res_type + " %" + GetParam().lhs + + " %" + GetParam().rhs + R"( + OpReturn + OpFunctionEnd + )"; + auto p = parser(test::Assemble(assembly)); + ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) + << p->error() << "\n" + << assembly; + FunctionEmitter fe(p.get(), *spirv_function(p.get(), 100)); + EXPECT_TRUE(fe.EmitBody()) << p->error(); + std::ostringstream ss; + ss << R"(VariableConst{ + x_1 + none + )" + << GetParam().expected; + auto got = ToString(p->builder(), fe.ast_body()); + EXPECT_THAT(got, HasSubstr(ss.str())) << "got:\n" << got << assembly; +} + INSTANTIATE_TEST_SUITE_P( SpvParserTest_ShiftLeftLogical, SpvBinaryBitTest, @@ -286,14 +329,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpBitwiseAnd", "int_40", "__i32", "ScalarConstructor[not set]{30}", "and", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpBitwiseAnd", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "and", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpBitwiseAnd", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "and", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpBitwiseAnd", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "and", @@ -301,15 +336,110 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpBitwiseAnd", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "and", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_BitwiseAnd_MixedSignedness, + SpvBinaryBitGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpBitwiseAnd", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + and + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpBitwiseAnd", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + and + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpBitwiseAnd", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + and + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpBitwiseAnd", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + and + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpBitwiseAnd", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "and", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpBitwiseAnd", + "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + and + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpBitwiseAnd", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "and", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpBitwiseAnd", + "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + and + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); INSTANTIATE_TEST_SUITE_P( SpvParserTest_BitwiseOr, @@ -323,14 +453,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpBitwiseOr", "int_40", "__i32", "ScalarConstructor[not set]{30}", "or", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpBitwiseOr", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "or", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpBitwiseOr", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "or", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpBitwiseOr", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "or", @@ -338,15 +460,109 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpBitwiseOr", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "or", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_BitwiseOr_MixedSignedness, + SpvBinaryBitGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpBitwiseOr", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + or + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpBitwiseOr", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + or + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpBitwiseOr", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + or + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpBitwiseOr", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + or + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpBitwiseOr", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "or", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpBitwiseOr", + "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + or + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpBitwiseOr", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "or", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpBitwiseOr", "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + or + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); INSTANTIATE_TEST_SUITE_P( SpvParserTest_BitwiseXor, @@ -360,14 +576,6 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"int", "int_30", "OpBitwiseXor", "int_40", "__i32", "ScalarConstructor[not set]{30}", "xor", "ScalarConstructor[not set]{40}"}, - // Mixed, returning uint - BinaryData{"uint", "int_30", "OpBitwiseXor", "uint_10", "__u32", - "ScalarConstructor[not set]{30}", "xor", - "ScalarConstructor[not set]{10}"}, - // Mixed, returning int - BinaryData{"int", "int_30", "OpBitwiseXor", "uint_10", "__i32", - "ScalarConstructor[not set]{30}", "xor", - "ScalarConstructor[not set]{10}"}, // Both v2uint BinaryData{"v2uint", "v2uint_10_20", "OpBitwiseXor", "v2uint_20_10", "__vec_2__u32", AstFor("v2uint_10_20"), "xor", @@ -375,15 +583,110 @@ INSTANTIATE_TEST_SUITE_P( // Both v2int BinaryData{"v2int", "v2int_30_40", "OpBitwiseXor", "v2int_40_30", "__vec_2__i32", AstFor("v2int_30_40"), "xor", - AstFor("v2int_40_30")}, + AstFor("v2int_40_30")})); + +INSTANTIATE_TEST_SUITE_P( + SpvParserTest_BitwiseXor_MixedSignedness, + SpvBinaryBitGeneralTest, + ::testing::Values( + // Mixed, uint <- int uint + BinaryDataGeneral{"uint", "int_30", "OpBitwiseXor", "uint_10", + R"(__u32 + { + Bitcast[not set]<__u32>{ + Binary[not set]{ + ScalarConstructor[not set]{30} + xor + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + } + })"}, + // Mixed, int <- int uint + BinaryDataGeneral{"int", "int_30", "OpBitwiseXor", "uint_10", + R"(__i32 + { + Binary[not set]{ + ScalarConstructor[not set]{30} + xor + Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + } + } + })"}, + // Mixed, uint <- uint int + BinaryDataGeneral{"uint", "uint_10", "OpBitwiseXor", "int_30", + R"(__u32 + { + Binary[not set]{ + ScalarConstructor[not set]{10} + xor + Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{30} + } + } + })"}, + // Mixed, int <- uint uint + BinaryDataGeneral{"int", "uint_20", "OpBitwiseXor", "uint_10", + R"(__i32 + { + Bitcast[not set]<__i32>{ + Binary[not set]{ + ScalarConstructor[not set]{20} + xor + ScalarConstructor[not set]{10} + } + } + })"}, // Mixed, returning v2uint - BinaryData{"v2uint", "v2int_30_40", "OpBitwiseXor", "v2uint_10_20", - "__vec_2__u32", AstFor("v2int_30_40"), "xor", - AstFor("v2uint_10_20")}, + BinaryDataGeneral{"v2uint", "v2int_30_40", "OpBitwiseXor", + "v2uint_10_20", + R"(__vec_2__u32 + { + Bitcast[not set]<__vec_2__u32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{30} + ScalarConstructor[not set]{40} + } + xor + Bitcast[not set]<__vec_2__i32>{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + } + } + } + })"}, // Mixed, returning v2int - BinaryData{"v2int", "v2int_40_30", "OpBitwiseXor", "v2uint_20_10", - "__vec_2__i32", AstFor("v2int_40_30"), "xor", - AstFor("v2uint_20_10")})); + BinaryDataGeneral{"v2int", "v2uint_10_20", "OpBitwiseXor", + "v2int_40_30", + R"(__vec_2__i32 + { + Bitcast[not set]<__vec_2__i32>{ + Binary[not set]{ + TypeConstructor[not set]{ + __vec_2__u32 + ScalarConstructor[not set]{10} + ScalarConstructor[not set]{20} + } + xor + Bitcast[not set]<__vec_2__u32>{ + TypeConstructor[not set]{ + __vec_2__i32 + ScalarConstructor[not set]{40} + ScalarConstructor[not set]{30} + } + } + } + } + })"} + + )); TEST_F(SpvUnaryBitTest, Not_Int_Int) { const auto assembly = CommonTypes() + R"( diff --git a/src/reader/spirv/function_logical_test.cc b/src/reader/spirv/function_logical_test.cc index 134cbdcfb1..a8e23c512c 100644 --- a/src/reader/spirv/function_logical_test.cc +++ b/src/reader/spirv/function_logical_test.cc @@ -300,19 +300,31 @@ INSTANTIATE_TEST_SUITE_P( SpvParserTest_IEqual, SpvBinaryLogicalTest, ::testing::Values( - // Both uint + // uint uint BinaryData{"bool", "uint_10", "OpIEqual", "uint_20", "__bool", "ScalarConstructor[not set]{10}", "equal", "ScalarConstructor[not set]{20}"}, - // Both int + // int int BinaryData{"bool", "int_30", "OpIEqual", "int_40", "__bool", "ScalarConstructor[not set]{30}", "equal", "ScalarConstructor[not set]{40}"}, - // Both v2uint + // uint int + BinaryData{"bool", "uint_10", "OpIEqual", "int_40", "__bool", + "ScalarConstructor[not set]{10}", "equal", + R"(Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{40} + })"}, + // int uint + BinaryData{"bool", "int_40", "OpIEqual", "uint_10", "__bool", + "ScalarConstructor[not set]{40}", "equal", + R"(Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + })"}, + // v2uint v2uint BinaryData{"v2bool", "v2uint_10_20", "OpIEqual", "v2uint_20_10", "__vec_2__bool", AstFor("v2uint_10_20"), "equal", AstFor("v2uint_20_10")}, - // Both v2int + // v2int v2int BinaryData{"v2bool", "v2int_30_40", "OpIEqual", "v2int_40_30", "__vec_2__bool", AstFor("v2int_30_40"), "equal", AstFor("v2int_40_30")})); @@ -340,6 +352,18 @@ INSTANTIATE_TEST_SUITE_P( BinaryData{"bool", "int_30", "OpINotEqual", "int_40", "__bool", "ScalarConstructor[not set]{30}", "not_equal", "ScalarConstructor[not set]{40}"}, + // uint int + BinaryData{"bool", "uint_10", "OpINotEqual", "int_40", "__bool", + "ScalarConstructor[not set]{10}", "not_equal", + R"(Bitcast[not set]<__u32>{ + ScalarConstructor[not set]{40} + })"}, + // int uint + BinaryData{"bool", "int_40", "OpINotEqual", "uint_10", "__bool", + "ScalarConstructor[not set]{40}", "not_equal", + R"(Bitcast[not set]<__i32>{ + ScalarConstructor[not set]{10} + })"}, // Both v2uint BinaryData{"v2bool", "v2uint_10_20", "OpINotEqual", "v2uint_20_10", "__vec_2__bool", AstFor("v2uint_10_20"), "not_equal", diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc index 7478bcf924..b5c6eab5da 100644 --- a/src/reader/spirv/parser_impl.cc +++ b/src/reader/spirv/parser_impl.cc @@ -155,6 +155,29 @@ bool AssumesUnsignedOperands(GLSLstd450 extended_opcode) { return false; } +// Returns true if the corresponding WGSL operation requires +// the signedness of the second operand to match the signedness of the +// first operand, and it's not one of the OpU* or OpS* instructions. +// (Those are handled via MakeOperand.) +bool AssumesSecondOperandSignednessMatchesFirstOperand(SpvOp opcode) { + switch (opcode) { + // All the OpI* integer binary operations. + case SpvOpIAdd: + case SpvOpISub: + case SpvOpIMul: + case SpvOpIEqual: + case SpvOpINotEqual: + // All the bitwise integer binary operations. + case SpvOpBitwiseAnd: + case SpvOpBitwiseOr: + case SpvOpBitwiseXor: + return true; + default: + break; + } + return false; +} + // Returns true if the corresponding WGSL operation requires // the signedness of the result to match the signedness of the first operand. bool AssumesResultSignednessMatchesFirstOperand(SpvOp opcode) { @@ -166,6 +189,12 @@ bool AssumesResultSignednessMatchesFirstOperand(SpvOp opcode) { case SpvOpSDiv: case SpvOpSMod: case SpvOpSRem: + case SpvOpIAdd: + case SpvOpISub: + case SpvOpIMul: + case SpvOpBitwiseAnd: + case SpvOpBitwiseOr: + case SpvOpBitwiseXor: return true; default: break; @@ -1536,6 +1565,21 @@ TypedExpression ParserImpl::RectifyOperandSignedness( return std::move(expr); } +TypedExpression ParserImpl::RectifySecondOperandSignedness( + const spvtools::opt::Instruction& inst, + type::Type* first_operand_type, + TypedExpression&& second_operand_expr) { + if ((first_operand_type != second_operand_expr.type) && + AssumesSecondOperandSignednessMatchesFirstOperand(inst.opcode())) { + // Conversion is required. + return {first_operand_type, + create(Source{}, first_operand_type, + second_operand_expr.expr)}; + } + // No conversion necessary. + return std::move(second_operand_expr); +} + type::Type* ParserImpl::ForcedResultType(const spvtools::opt::Instruction& inst, type::Type* first_operand_type) { const auto opcode = inst.opcode(); diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h index 4ebc47ae73..d645b8cf19 100644 --- a/src/reader/spirv/parser_impl.h +++ b/src/reader/spirv/parser_impl.h @@ -327,6 +327,19 @@ class ParserImpl : Reader { const spvtools::opt::Instruction& inst, TypedExpression&& expr); + /// Conversts a second operand to the signedness of the first operand + /// of a binary operator, if the WGSL operator requires they be the same. + /// Returns the converted expression, or the original expression if the + /// conversion is not needed. + /// @param inst the SPIR-V instruction + /// @param first_operand_type the type of the first operand to the instruction + /// @param second_operand_expr the second operand of the instruction + /// @returns second_operand_expr, or a cast of it + TypedExpression RectifySecondOperandSignedness( + const spvtools::opt::Instruction& inst, + type::Type* first_operand_type, + TypedExpression&& second_operand_expr); + /// Returns the "forced" result type for the given SPIR-V instruction. /// If the WGSL result type for an operation has a more strict rule than /// requried by SPIR-V, then we say the result type is "forced". This occurs