From 1c94938726db859f555167d5028f5230d83a0567 Mon Sep 17 00:00:00 2001 From: Antonio Maiorano Date: Wed, 19 Oct 2022 17:46:12 +0000 Subject: [PATCH] tint: implement updated spec rules for shift left of concrete values Rules for shift left of concrete values are now split between signed and unsigned. Shifting unsigned values no longer fails with "sign change" errors. Furthermore, shifting unsigned values must only shift out 0s. See https://github.com/gpuweb/gpuweb/pull/3539. Bug: tint:1701 Bug: tint:1717 Change-Id: Iba2799f4b02cdc77cc58a6c7c104aaa408f0f0f9 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/106381 Reviewed-by: Dan Sinclair Kokoro: Kokoro Commit-Queue: Antonio Maiorano --- src/tint/resolver/const_eval.cc | 26 +++++-- .../resolver/const_eval_binary_op_test.cc | 69 ++++++++++++++----- src/tint/resolver/const_eval_test.h | 2 +- test/tint/bug/tint/1717.wgsl | 3 + .../tint/bug/tint/1717.wgsl.expected.dxc.hlsl | 8 +++ .../tint/bug/tint/1717.wgsl.expected.fxc.hlsl | 8 +++ test/tint/bug/tint/1717.wgsl.expected.glsl | 10 +++ test/tint/bug/tint/1717.wgsl.expected.msl | 7 ++ test/tint/bug/tint/1717.wgsl.expected.spvasm | 24 +++++++ test/tint/bug/tint/1717.wgsl.expected.wgsl | 3 + 10 files changed, 135 insertions(+), 25 deletions(-) create mode 100644 test/tint/bug/tint/1717.wgsl create mode 100644 test/tint/bug/tint/1717.wgsl.expected.dxc.hlsl create mode 100644 test/tint/bug/tint/1717.wgsl.expected.fxc.hlsl create mode 100644 test/tint/bug/tint/1717.wgsl.expected.glsl create mode 100644 test/tint/bug/tint/1717.wgsl.expected.msl create mode 100644 test/tint/bug/tint/1717.wgsl.expected.spvasm create mode 100644 test/tint/bug/tint/1717.wgsl.expected.wgsl diff --git a/src/tint/resolver/const_eval.cc b/src/tint/resolver/const_eval.cc index c5cce2de83..a5b541e9a2 100644 --- a/src/tint/resolver/const_eval.cc +++ b/src/tint/resolver/const_eval.cc @@ -1491,13 +1491,25 @@ ConstEval::Result ConstEval::OpShiftLeft(const sem::Type* ty, return nullptr; } - // The e2 + 1 most significant bits of e1 must have the same bit value, otherwise - // sign change (overflow) would occur. - size_t must_match_msb = e2u + 1; - UT mask = ~UT{0} << (bit_width - must_match_msb); - if ((e1u & mask) != 0 && (e1u & mask) != mask) { - AddError("shift left operation results in sign change", source); - return nullptr; + if constexpr (std::is_signed_v) { + // If T is a signed integer type, and the e2+1 most significant bits of e1 do + // not have the same bit value, then error. + size_t must_match_msb = e2u + 1; + UT mask = ~UT{0} << (bit_width - must_match_msb); + if ((e1u & mask) != 0 && (e1u & mask) != mask) { + AddError("shift left operation results in sign change", source); + return nullptr; + } + } else { + // If T is an unsigned integer type, and any of the e2 most significant bits of + // e1 are 1, then error. + if (e2u > 0) { + size_t must_be_zero_msb = e2u; + UT mask = ~UT{0} << (bit_width - must_be_zero_msb); + if ((e1u & mask) != 0) { + AddError(OverflowErrorMessage(e1, "<<", e2), source); + } + } } } diff --git a/src/tint/resolver/const_eval_binary_op_test.cc b/src/tint/resolver/const_eval_binary_op_test.cc index efd3adcfee..b45401e65e 100644 --- a/src/tint/resolver/const_eval_binary_op_test.cc +++ b/src/tint/resolver/const_eval_binary_op_test.cc @@ -611,19 +611,16 @@ std::vector ShiftLeftCases() { using ST = std::conditional_t, T, u32>; using B = BitValues; auto r = std::vector{ - C(T{0b1010}, ST{0}, T{0b0000'0000'1010}), // - C(T{0b1010}, ST{1}, T{0b0000'0001'0100}), // - C(T{0b1010}, ST{2}, T{0b0000'0010'1000}), // - C(T{0b1010}, ST{3}, T{0b0000'0101'0000}), // - C(T{0b1010}, ST{4}, T{0b0000'1010'0000}), // - C(T{0b1010}, ST{5}, T{0b0001'0100'0000}), // - C(T{0b1010}, ST{6}, T{0b0010'1000'0000}), // - C(T{0b1010}, ST{7}, T{0b0101'0000'0000}), // - C(T{0b1010}, ST{8}, T{0b1010'0000'0000}), // - C(B::LeftMost, ST{0}, B::LeftMost), // - C(B::TwoLeftMost, ST{1}, B::LeftMost), // No overflow - C(B::All, ST{1}, B::AllButRightMost), // No overflow - C(B::All, ST{B::NumBits - 1}, B::LeftMost), // No overflow + C(T{0b1010}, ST{0}, T{0b0000'0000'1010}), // + C(T{0b1010}, ST{1}, T{0b0000'0001'0100}), // + C(T{0b1010}, ST{2}, T{0b0000'0010'1000}), // + C(T{0b1010}, ST{3}, T{0b0000'0101'0000}), // + C(T{0b1010}, ST{4}, T{0b0000'1010'0000}), // + C(T{0b1010}, ST{5}, T{0b0001'0100'0000}), // + C(T{0b1010}, ST{6}, T{0b0010'1000'0000}), // + C(T{0b1010}, ST{7}, T{0b0101'0000'0000}), // + C(T{0b1010}, ST{8}, T{0b1010'0000'0000}), // + C(B::LeftMost, ST{0}, B::LeftMost), // C(Vec(T{0b1010}, T{0b1010}), // Vec(ST{0}, ST{1}), // @@ -641,7 +638,7 @@ std::vector ShiftLeftCases() { // Only abstract 0 can be shifted left as much as we like. For concrete 0 (and any number), it // cannot be shifted equal or more than the number of bits of the lhs (see - // ResolverConstEvalShiftLeftConcreteGeqBitWidthError) + // ResolverConstEvalShiftLeftConcreteGeqBitWidthError for negative tests) ConcatIntoIf>( // r, std::vector{ C(T{0}, ST{64}, T{0}), @@ -656,6 +653,31 @@ std::vector ShiftLeftCases() { C(Negate(T{0}), T::Highest(), Negate(T{0})), }); + // Cases that are fine for signed values (no sign change), but would overflow unsigned values. + // See ResolverConstEvalBinaryOpTest_Overflow for negative tests. + ConcatIntoIf>( // + r, std::vector{ + C(B::TwoLeftMost, ST{1}, B::LeftMost), // + C(B::All, ST{1}, B::AllButRightMost), // + C(B::All, ST{B::NumBits - 1}, B::LeftMost) // + }); + + // Cases that are fine for unsigned values, but would overflow (sign change) signed + // values. See ShiftLeftSignChangeErrorCases() for negative tests. + ConcatIntoIf>( // + r, std::vector{ + C(T{0b0001}, ST{B::NumBits - 1}, B::Lsh(0b0001, B::NumBits - 1)), + C(T{0b0010}, ST{B::NumBits - 2}, B::Lsh(0b0010, B::NumBits - 2)), + C(T{0b0100}, ST{B::NumBits - 3}, B::Lsh(0b0100, B::NumBits - 3)), + C(T{0b1000}, ST{B::NumBits - 4}, B::Lsh(0b1000, B::NumBits - 4)), + + C(T{0b0011}, ST{B::NumBits - 2}, B::Lsh(0b0011, B::NumBits - 2)), + C(T{0b0110}, ST{B::NumBits - 3}, B::Lsh(0b0110, B::NumBits - 3)), + C(T{0b1100}, ST{B::NumBits - 4}, B::Lsh(0b1100, B::NumBits - 4)), + + C(B::AllButLeftMost, ST{1}, B::AllButRightMost), + }); + return r; } INSTANTIATE_TEST_SUITE_P(ShiftLeft, @@ -795,7 +817,21 @@ INSTANTIATE_TEST_SUITE_P( Val(AInt{BitValues::NumBits + 1})}, // OverflowCase{ast::BinaryOp::kShiftLeft, // Val(AInt{BitValues::AllButLeftMost}), // - Val(AInt{BitValues::NumBits + 1000})} + Val(AInt{BitValues::NumBits + 1000})}, + + // ShiftLeft of u32s that overflow (non-zero bits get shifted out) + OverflowCase{ast::BinaryOp::kShiftLeft, Val(0b00010_u), Val(31_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(0b00100_u), Val(30_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(0b01000_u), Val(29_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(0b10000_u), Val(28_u)}, + // ... + OverflowCase{ast::BinaryOp::kShiftLeft, Val(u32(1u << 28)), Val(4_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(u32(1u << 29)), Val(3_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(u32(1u << 30)), Val(2_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(u32(1u << 31)), Val(1_u)}, + // And some more + OverflowCase{ast::BinaryOp::kShiftLeft, Val(BitValues::All), Val(1_u)}, + OverflowCase{ast::BinaryOp::kShiftLeft, Val(BitValues::AllButLeftMost), Val(2_u)} )); @@ -945,8 +981,7 @@ INSTANTIATE_TEST_SUITE_P(Test, ResolverConstEvalShiftLeftSignChangeError, testing::ValuesIn(Concat( // ShiftLeftSignChangeErrorCases(), - ShiftLeftSignChangeErrorCases(), - ShiftLeftSignChangeErrorCases()))); + ShiftLeftSignChangeErrorCases()))); } // namespace } // namespace tint::resolver diff --git a/src/tint/resolver/const_eval_test.h b/src/tint/resolver/const_eval_test.h index 2761daff0c..1915c05e52 100644 --- a/src/tint/resolver/const_eval_test.h +++ b/src/tint/resolver/const_eval_test.h @@ -281,7 +281,7 @@ struct BitValues { /// @returns the shifted value template static constexpr NumberT Lsh(U val, V shiftBy) { - return NumberT{T{val} << T{shiftBy}}; + return NumberT{static_cast(val) << static_cast(shiftBy)}; } }; diff --git a/test/tint/bug/tint/1717.wgsl b/test/tint/bug/tint/1717.wgsl new file mode 100644 index 0000000000..3f42364f64 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl @@ -0,0 +1,3 @@ +fn f() -> u32 { + return 0x1u << 31u; +} diff --git a/test/tint/bug/tint/1717.wgsl.expected.dxc.hlsl b/test/tint/bug/tint/1717.wgsl.expected.dxc.hlsl new file mode 100644 index 0000000000..e9ac899f36 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.dxc.hlsl @@ -0,0 +1,8 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +uint f() { + return 2147483648u; +} diff --git a/test/tint/bug/tint/1717.wgsl.expected.fxc.hlsl b/test/tint/bug/tint/1717.wgsl.expected.fxc.hlsl new file mode 100644 index 0000000000..e9ac899f36 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.fxc.hlsl @@ -0,0 +1,8 @@ +[numthreads(1, 1, 1)] +void unused_entry_point() { + return; +} + +uint f() { + return 2147483648u; +} diff --git a/test/tint/bug/tint/1717.wgsl.expected.glsl b/test/tint/bug/tint/1717.wgsl.expected.glsl new file mode 100644 index 0000000000..ff2f115304 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.glsl @@ -0,0 +1,10 @@ +#version 310 es + +layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in; +void unused_entry_point() { + return; +} +uint f() { + return 2147483648u; +} + diff --git a/test/tint/bug/tint/1717.wgsl.expected.msl b/test/tint/bug/tint/1717.wgsl.expected.msl new file mode 100644 index 0000000000..4ff2ed2af3 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.msl @@ -0,0 +1,7 @@ +#include + +using namespace metal; +uint f() { + return 2147483648u; +} + diff --git a/test/tint/bug/tint/1717.wgsl.expected.spvasm b/test/tint/bug/tint/1717.wgsl.expected.spvasm new file mode 100644 index 0000000000..ca8aab399b --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.spvasm @@ -0,0 +1,24 @@ +; SPIR-V +; Version: 1.3 +; Generator: Google Tint Compiler; 0 +; Bound: 10 +; Schema: 0 + OpCapability Shader + OpMemoryModel Logical GLSL450 + OpEntryPoint GLCompute %unused_entry_point "unused_entry_point" + OpExecutionMode %unused_entry_point LocalSize 1 1 1 + OpName %unused_entry_point "unused_entry_point" + OpName %f "f" + %void = OpTypeVoid + %1 = OpTypeFunction %void + %uint = OpTypeInt 32 0 + %5 = OpTypeFunction %uint +%uint_2147483648 = OpConstant %uint 2147483648 +%unused_entry_point = OpFunction %void None %1 + %4 = OpLabel + OpReturn + OpFunctionEnd + %f = OpFunction %uint None %5 + %8 = OpLabel + OpReturnValue %uint_2147483648 + OpFunctionEnd diff --git a/test/tint/bug/tint/1717.wgsl.expected.wgsl b/test/tint/bug/tint/1717.wgsl.expected.wgsl new file mode 100644 index 0000000000..e87bef3ef1 --- /dev/null +++ b/test/tint/bug/tint/1717.wgsl.expected.wgsl @@ -0,0 +1,3 @@ +fn f() -> u32 { + return (1u << 31u); +}