[llvm] [WebAssembly] Don't fold non-nuw add/sub in FastISel (PR #111278)
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 5 22:24:50 PDT 2024
https://github.com/aheejin created https://github.com/llvm/llvm-project/pull/111278
We should not fold one of add/sub operands into a load/store's offset when `nuw` (no unsigned wrap) is not present, because the address calculation, which adds the offset with the operand, does not wrap.
This is handled correctly in the normal ISel:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp#L328-L332 but not in FastISel.
This positivity check in FastISel is not sufficient to avoid this case fully:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L348-L352
because
1. Even if RHS is within signed int range, depending on the value of the LHS, the resulting value can exceed uint32 max.
2. When one of the operands is a label, `Address` can contain a `GlobalValue` and a `Reg` at the same time, so the `GlobalValue` becomes incorrectly an offset: https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L53-L69 https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L409-L417
Both cases are in the newly added test.
This PR bails out of FastISel when there is an add/sub without a `nuw`.
We should handle `SUB` too because `SUB` is the same as `ADD` when RHS's sign changes. I checked why our current normal ISel only handles `ADD`, and the reason it's OK for the normal ISel to handle only `ADD` seems that DAGCombiner replaces `SUB` with `ADD` here: https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L3904-L3907
Fixes #111018.
>From 1f8f7d5b400403254ca889db879c1887fc68e152 Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Sat, 5 Oct 2024 07:01:53 +0000
Subject: [PATCH] [WebAssembly] Don't fold non-nuw add/sub in FastISel
We should not fold one of add/sub operands into a load/store's offset
when `nuw` (no unsigned wrap) is not present, because the address
calculation, which adds the offset with the operand, does not wrap.
This is handled correctly in the normal ISel:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp#L328-L332
but not in FastISel.
This positivity check in FastISel is not sufficient to avoid this case
fully:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L348-L352
because
1. Even if RHS is within signed int range, depending on the value of the
LHS, the resulting value can exceed uint32 max.
2. When one of the operands is a label, `Address` can contain a
`GlobalValue` and a `Reg` at the same time, so the `GlobalValue`
becomes incorrectly an offset:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L53-L69
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L409-L417
Both cases are in the newly added test.
This PR bails out of FastISel when there is an add/sub without a `nuw`.
We should handle `SUB` too because `SUB` is the same as `ADD` when
RHS's sign changes. I checked why our current normal ISel only handles
`ADD`, and the reason it's OK for the normal ISel to handle only `ADD`
seems that DAGCombiner replaces `SUB` with `ADD` here:
https://github.com/llvm/llvm-project/blob/6de5305b3d7a4a19a29b35d481a8090e2a6d3a7e/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L3904-L3907
Fixes #111018.
---
.../WebAssembly/WebAssemblyFastISel.cpp | 12 ++
.../CodeGen/WebAssembly/fast-isel-bailout.ll | 106 ++++++++++++++++++
.../CodeGen/WebAssembly/fast-isel-pr47040.ll | 2 +-
3 files changed, 119 insertions(+), 1 deletion(-)
create mode 100644 llvm/test/CodeGen/WebAssembly/fast-isel-bailout.ll
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
index 317c6463985dcb..b004192eed6dcc 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
@@ -337,6 +337,12 @@ bool WebAssemblyFastISel::computeAddress(const Value *Obj, Address &Addr) {
break;
}
case Instruction::Add: {
+ // We should not fold operands into an offset when 'nuw' (no unsigned wrap)
+ // is not present, because the address calculation does not wrap.
+ if (auto *OFBinOp = dyn_cast<OverflowingBinaryOperator>(U))
+ if (!OFBinOp->hasNoUnsignedWrap())
+ return false;
+
// Adds of constants are common and easy enough.
const Value *LHS = U->getOperand(0);
const Value *RHS = U->getOperand(1);
@@ -360,6 +366,12 @@ bool WebAssemblyFastISel::computeAddress(const Value *Obj, Address &Addr) {
break;
}
case Instruction::Sub: {
+ // We should not fold operands into an offset when 'nuw' (no unsigned wrap)
+ // is not present, because the address calculation does not wrap.
+ if (auto *OFBinOp = dyn_cast<OverflowingBinaryOperator>(U))
+ if (!OFBinOp->hasNoUnsignedWrap())
+ return false;
+
// Subs of constants are common and easy enough.
const Value *LHS = U->getOperand(0);
const Value *RHS = U->getOperand(1);
diff --git a/llvm/test/CodeGen/WebAssembly/fast-isel-bailout.ll b/llvm/test/CodeGen/WebAssembly/fast-isel-bailout.ll
new file mode 100644
index 00000000000000..80c5c31718e744
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/fast-isel-bailout.ll
@@ -0,0 +1,106 @@
+; RUN: llc < %s -asm-verbose=false -fast-isel -verify-machineinstrs | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+; FastISel should not fold one of the add/sub operands into a load/store's
+; offset when 'nuw' (no unsigned wrap) is not present, because the address
+; calculation does not wrap. When there is an add/sub and nuw is not present, we
+; bail out of FastISel.
+
+ at mylabel = external global ptr
+
+; CHECK-LABEL: dont_fold_non_nuw_add_load:
+; CHECK: local.get 0
+; CHECK-NEXT: i32.const 2147483644
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.load 0
+define i32 @dont_fold_non_nuw_add_load(ptr %p) {
+ %q = ptrtoint ptr %p to i32
+ %r = add i32 %q, 2147483644
+ %s = inttoptr i32 %r to ptr
+ %t = load i32, ptr %s
+ ret i32 %t
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_add_store:
+; CHECK: local.get 0
+; CHECK-NEXT: i32.const 2147483644
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.const 5
+; CHECK-NEXT: i32.store 0
+define void @dont_fold_non_nuw_add_store(ptr %p) {
+ %q = ptrtoint ptr %p to i32
+ %r = add i32 %q, 2147483644
+ %s = inttoptr i32 %r to ptr
+ store i32 5, ptr %s
+ ret void
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_add_load_2:
+; CHECK: i32.const mylabel
+; CHECK-NEXT: i32.const -4
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.load 0
+define i32 @dont_fold_non_nuw_add_load_2() {
+ %t = load i32, ptr inttoptr (i32 add (i32 ptrtoint (ptr @mylabel to i32), i32 -4) to ptr), align 4
+ ret i32 %t
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_add_store_2:
+; CHECK: i32.const mylabel
+; CHECK-NEXT: i32.const -4
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.const 5
+; CHECK-NEXT: i32.store 0
+define void @dont_fold_non_nuw_add_store_2() {
+ store i32 5, ptr inttoptr (i32 add (i32 ptrtoint (ptr @mylabel to i32), i32 -4) to ptr), align 4
+ ret void
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_sub_load:
+; CHECK: local.get 0
+; CHECK-NEXT: i32.const 2147483644
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.load 0
+define i32 @dont_fold_non_nuw_sub_load(ptr %p) {
+ %q = ptrtoint ptr %p to i32
+ %r = sub i32 %q, -2147483644
+ %s = inttoptr i32 %r to ptr
+ %t = load i32, ptr %s
+ ret i32 %t
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_sub_store:
+; CHECK: local.get 0
+; CHECK-NEXT: i32.const 2147483644
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.const 5
+; CHECK-NEXT: i32.store 0
+define void @dont_fold_non_nuw_sub_store(ptr %p) {
+ %q = ptrtoint ptr %p to i32
+ %r = sub i32 %q, -2147483644
+ %s = inttoptr i32 %r to ptr
+ store i32 5, ptr %s
+ ret void
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_sub_load_2:
+; CHECK: i32.const mylabel
+; CHECK-NEXT: i32.const -4
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.load 0
+define i32 @dont_fold_non_nuw_sub_load_2() {
+ %t = load i32, ptr inttoptr (i32 sub (i32 ptrtoint (ptr @mylabel to i32), i32 4) to ptr), align 4
+ ret i32 %t
+}
+
+; CHECK-LABEL: dont_fold_non_nuw_sub_store_2:
+; CHECK: i32.const mylabel
+; CHECK-NEXT: i32.const -4
+; CHECK-NEXT: i32.add
+; CHECK-NEXT: i32.const 5
+; CHECK-NEXT: i32.store 0
+define void @dont_fold_non_nuw_sub_store_2() {
+ store i32 5, ptr inttoptr (i32 sub (i32 ptrtoint (ptr @mylabel to i32), i32 4) to ptr), align 4
+ ret void
+}
diff --git a/llvm/test/CodeGen/WebAssembly/fast-isel-pr47040.ll b/llvm/test/CodeGen/WebAssembly/fast-isel-pr47040.ll
index 6a1304cb9a93a8..75cb5b66b3ebee 100644
--- a/llvm/test/CodeGen/WebAssembly/fast-isel-pr47040.ll
+++ b/llvm/test/CodeGen/WebAssembly/fast-isel-pr47040.ll
@@ -14,7 +14,7 @@ target triple = "wasm32-unknown-unknown"
define i32 @foo() {
%stack_addr = alloca i32
%stack_i = ptrtoint ptr %stack_addr to i32
- %added = add i32 %stack_i, undef
+ %added = add nuw i32 %stack_i, undef
%added_addr = inttoptr i32 %added to ptr
%ret = load i32, ptr %added_addr
ret i32 %ret
More information about the llvm-commits
mailing list