Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WebAssembly] Select BUILD_VECTOR with large unsigned lane values #85880

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

tlively
Copy link
Collaborator

@tlively tlively commented Mar 20, 2024

Previously we expected lane constants to be in the range of signed values for each lane size, but the included test case produced large unsigned values that fall outside that range. Allow instruction selection to proceed in this case rather than failing.

Fixes #63817.

Previously we expected lane constants to be in the range of signed values for
each lane size, but the included test case produced large unsigned values that
fall outside that range. Allow instruction selection to proceed in this case
rather than failing.

Fixes llvm#63817.
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 20, 2024

@llvm/pr-subscribers-backend-webassembly

Author: Thomas Lively (tlively)

Changes

Previously we expected lane constants to be in the range of signed values for each lane size, but the included test case produced large unsigned values that fall outside that range. Allow instruction selection to proceed in this case rather than failing.

Fixes #63817.


Full diff: https://github.com/llvm/llvm-project/pull/85880.diff

2 Files Affected:

  • (modified) llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td (+4-2)
  • (added) llvm/test/CodeGen/WebAssembly/pr63817.ll (+15)
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td b/llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
index 8cd41d7017a023..35f7d49be7a965 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
@@ -46,10 +46,12 @@ defm "" : ARGUMENT<V128, v2i64>;
 defm "" : ARGUMENT<V128, v4f32>;
 defm "" : ARGUMENT<V128, v2f64>;
 
-// Constrained immediate argument types
+// Constrained immediate argument types. Allow any value from the minimum signed
+// value to the maximum unsigned value for the lane size.
 foreach SIZE = [8, 16] in
 def ImmI#SIZE : ImmLeaf<i32,
-  "return -(1 << ("#SIZE#" - 1)) <= Imm && Imm < (1 << ("#SIZE#" - 1));"
+  // -2^(n-1) <= Imm <= 2^n-1, avoiding UB when n == 64.
+  "return -(1 << ("#SIZE#" - 1)) <= Imm && Imm <= int64_t(uint64_t(1 << ("#SIZE#" - 1) << 1) - 1);"
 >;
 foreach SIZE = [2, 4, 8, 16, 32] in
 def LaneIdx#SIZE : ImmLeaf<i32, "return 0 <= Imm && Imm < "#SIZE#";">;
diff --git a/llvm/test/CodeGen/WebAssembly/pr63817.ll b/llvm/test/CodeGen/WebAssembly/pr63817.ll
new file mode 100644
index 00000000000000..252768d43f1888
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/pr63817.ll
@@ -0,0 +1,15 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=wasm32 -mattr=+simd128 | FileCheck %s
+
+;; Regression test for a bug in which BUILD_VECTOR nodes with large unsigned
+;; lane constants were not properly selected.
+define <4 x i8> @test(<4 x i8> %0) {
+; CHECK-LABEL: test:
+; CHECK:         .functype test (v128) -> (v128)
+; CHECK-NEXT:  # %bb.0:
+; CHECK-NEXT:    v128.const 255, 17, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
+; CHECK-NEXT:    # fallthrough-return
+  %V1 = or <4 x i8> <i8 255, i8 255, i8 255, i8 255>, %0
+  %V2 = insertelement <4 x i8> %V1, i8 17, i32 1
+  ret <4 x i8> %V2
+}

@tlively
Copy link
Collaborator Author

tlively commented Mar 20, 2024

@aheejin, this is a re-upload of https://reviews.llvm.org/D155386 with fixes.

@DataCorrupted DataCorrupted self-requested a review March 20, 2024 00:36
foreach SIZE = [8, 16] in
def ImmI#SIZE : ImmLeaf<i32,
"return -(1 << ("#SIZE#" - 1)) <= Imm && Imm < (1 << ("#SIZE#" - 1));"
// -2^(n-1) <= Imm <= 2^n-1, avoiding UB when n == 64.
"return -(1 << ("#SIZE#" - 1)) <= Imm && Imm <= int64_t(uint64_t(1 << ("#SIZE#" - 1) << 1) - 1);"
Copy link
Member

@aheejin aheejin Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why do we need 1 << ("#SIZE#" - 1) << 1? Can't we just use 1 << "#SIZE#"?
  • Can you elaborate what uint64_t and int64_t casting do and what is the UB about?
  • Why do we need to consider n == 64, give that this is under foreach SIZE = [8, 16]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, I completely missed that it says SIZE can only be 8 or 16 right there. Simplifications incoming!

None of this matters any more, but to answer your questions:

  • Why do we need 1 << ("#SIZE#" - 1) << 1? Can't we just use 1 << "#SIZE#"?

Shifting equal to or greater than the number of bits is UB, so 1 << SIZE would be UB if size were 64. The workaround is to do 1 << 63 << 1.

  • Can you elaborate what uint64_t and int64_t casting do and what is the UB about?

Now that I think about this more, this was never necessary. I was concerned about UB due to underflow on the subtraction, but subtracting 1 from 0 is perfectly fine.

@tlively tlively requested a review from aheejin March 20, 2024 02:19
@tlively tlively merged commit 767e0c8 into llvm:main Mar 20, 2024
4 checks passed
@tlively tlively deleted the D155386 branch March 20, 2024 15:42
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…vm#85880)

Previously we expected lane constants to be in the range of signed
values for each lane size, but the included test case produced large
unsigned values that fall outside that range. Allow instruction
selection to proceed in this case rather than failing.

Fixes llvm#63817.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WASM] Cannot select BUILD_VECTOR when enabling SIMD128
4 participants