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 <dsinclair@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
diff --git a/src/tint/resolver/const_eval.cc b/src/tint/resolver/const_eval.cc
index c5cce2d..a5b541e 100644
--- a/src/tint/resolver/const_eval.cc
+++ b/src/tint/resolver/const_eval.cc
@@ -1491,13 +1491,25 @@
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<T>) {
+ // 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 efd3adc..b45401e 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 @@
using ST = std::conditional_t<IsAbstract<T>, T, u32>;
using B = BitValues<T>;
auto r = std::vector<Case>{
- 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 @@
// 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<IsAbstract<T>>( //
r, std::vector<Case>{
C(T{0}, ST{64}, T{0}),
@@ -656,6 +653,31 @@
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<IsSignedIntegral<T>>( //
+ r, std::vector<Case>{
+ 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<IsUnsignedIntegral<T>>( //
+ r, std::vector<Case>{
+ 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 @@
Val(AInt{BitValues<AInt>::NumBits + 1})}, //
OverflowCase{ast::BinaryOp::kShiftLeft, //
Val(AInt{BitValues<AInt>::AllButLeftMost}), //
- Val(AInt{BitValues<AInt>::NumBits + 1000})}
+ Val(AInt{BitValues<AInt>::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<u32>::All), Val(1_u)},
+ OverflowCase{ast::BinaryOp::kShiftLeft, Val(BitValues<u32>::AllButLeftMost), Val(2_u)}
));
@@ -945,8 +981,7 @@
ResolverConstEvalShiftLeftSignChangeError,
testing::ValuesIn(Concat( //
ShiftLeftSignChangeErrorCases<AInt>(),
- ShiftLeftSignChangeErrorCases<i32>(),
- ShiftLeftSignChangeErrorCases<u32>())));
+ ShiftLeftSignChangeErrorCases<i32>())));
} // namespace
} // namespace tint::resolver
diff --git a/src/tint/resolver/const_eval_test.h b/src/tint/resolver/const_eval_test.h
index 2761daf..1915c05 100644
--- a/src/tint/resolver/const_eval_test.h
+++ b/src/tint/resolver/const_eval_test.h
@@ -281,7 +281,7 @@
/// @returns the shifted value
template <typename U, typename V>
static constexpr NumberT Lsh(U val, V shiftBy) {
- return NumberT{T{val} << T{shiftBy}};
+ return NumberT{static_cast<T>(val) << static_cast<T>(shiftBy)};
}
};
diff --git a/test/tint/bug/tint/1717.wgsl b/test/tint/bug/tint/1717.wgsl
new file mode 100644
index 0000000..3f42364
--- /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 0000000..e9ac899
--- /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 0000000..e9ac899
--- /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 0000000..ff2f115
--- /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 0000000..4ff2ed2
--- /dev/null
+++ b/test/tint/bug/tint/1717.wgsl.expected.msl
@@ -0,0 +1,7 @@
+#include <metal_stdlib>
+
+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 0000000..ca8aab3
--- /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 0000000..e87bef3
--- /dev/null
+++ b/test/tint/bug/tint/1717.wgsl.expected.wgsl
@@ -0,0 +1,3 @@
+fn f() -> u32 {
+ return (1u << 31u);
+}
