Skip to content

Commit 1f8f7d5

Browse files
aheejinaheejin
committed
[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.
1 parent fda2fea commit 1f8f7d5

File tree

3 files changed

+119
-1
lines changed

3 files changed

+119
-1
lines changed

‎llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,6 +337,12 @@ bool WebAssemblyFastISel::computeAddress(const Value *Obj, Address &Addr) {
337337
break;
338338
}
339339
case Instruction::Add: {
340+
// We should not fold operands into an offset when 'nuw' (no unsigned wrap)
341+
// is not present, because the address calculation does not wrap.
342+
if (auto *OFBinOp = dyn_cast<OverflowingBinaryOperator>(U))
343+
if (!OFBinOp->hasNoUnsignedWrap())
344+
return false;
345+
340346
// Adds of constants are common and easy enough.
341347
const Value *LHS = U->getOperand(0);
342348
const Value *RHS = U->getOperand(1);
@@ -360,6 +366,12 @@ bool WebAssemblyFastISel::computeAddress(const Value *Obj, Address &Addr) {
360366
break;
361367
}
362368
case Instruction::Sub: {
369+
// We should not fold operands into an offset when 'nuw' (no unsigned wrap)
370+
// is not present, because the address calculation does not wrap.
371+
if (auto *OFBinOp = dyn_cast<OverflowingBinaryOperator>(U))
372+
if (!OFBinOp->hasNoUnsignedWrap())
373+
return false;
374+
363375
// Subs of constants are common and easy enough.
364376
const Value *LHS = U->getOperand(0);
365377
const Value *RHS = U->getOperand(1);

‎llvm/test/CodeGen/WebAssembly/fast-isel-bailout.ll‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
; RUN: llc < %s -asm-verbose=false -fast-isel -verify-machineinstrs | FileCheck %s
2+
3+
target triple = "wasm32-unknown-unknown"
4+
5+
; FastISel should not fold one of the add/sub operands into a load/store's
6+
; offset when 'nuw' (no unsigned wrap) is not present, because the address
7+
; calculation does not wrap. When there is an add/sub and nuw is not present, we
8+
; bail out of FastISel.
9+
10+
@mylabel = external global ptr
11+
12+
; CHECK-LABEL: dont_fold_non_nuw_add_load:
13+
; CHECK: local.get 0
14+
; CHECK-NEXT: i32.const 2147483644
15+
; CHECK-NEXT: i32.add
16+
; CHECK-NEXT: i32.load 0
17+
define i32 @dont_fold_non_nuw_add_load(ptr %p) {
18+
%q = ptrtoint ptr %p to i32
19+
%r = add i32 %q, 2147483644
20+
%s = inttoptr i32 %r to ptr
21+
%t = load i32, ptr %s
22+
ret i32 %t
23+
}
24+
25+
; CHECK-LABEL: dont_fold_non_nuw_add_store:
26+
; CHECK: local.get 0
27+
; CHECK-NEXT: i32.const 2147483644
28+
; CHECK-NEXT: i32.add
29+
; CHECK-NEXT: i32.const 5
30+
; CHECK-NEXT: i32.store 0
31+
define void @dont_fold_non_nuw_add_store(ptr %p) {
32+
%q = ptrtoint ptr %p to i32
33+
%r = add i32 %q, 2147483644
34+
%s = inttoptr i32 %r to ptr
35+
store i32 5, ptr %s
36+
ret void
37+
}
38+
39+
; CHECK-LABEL: dont_fold_non_nuw_add_load_2:
40+
; CHECK: i32.const mylabel
41+
; CHECK-NEXT: i32.const -4
42+
; CHECK-NEXT: i32.add
43+
; CHECK-NEXT: i32.load 0
44+
define i32 @dont_fold_non_nuw_add_load_2() {
45+
%t = load i32, ptr inttoptr (i32 add (i32 ptrtoint (ptr @mylabel to i32), i32 -4) to ptr), align 4
46+
ret i32 %t
47+
}
48+
49+
; CHECK-LABEL: dont_fold_non_nuw_add_store_2:
50+
; CHECK: i32.const mylabel
51+
; CHECK-NEXT: i32.const -4
52+
; CHECK-NEXT: i32.add
53+
; CHECK-NEXT: i32.const 5
54+
; CHECK-NEXT: i32.store 0
55+
define void @dont_fold_non_nuw_add_store_2() {
56+
store i32 5, ptr inttoptr (i32 add (i32 ptrtoint (ptr @mylabel to i32), i32 -4) to ptr), align 4
57+
ret void
58+
}
59+
60+
; CHECK-LABEL: dont_fold_non_nuw_sub_load:
61+
; CHECK: local.get 0
62+
; CHECK-NEXT: i32.const 2147483644
63+
; CHECK-NEXT: i32.add
64+
; CHECK-NEXT: i32.load 0
65+
define i32 @dont_fold_non_nuw_sub_load(ptr %p) {
66+
%q = ptrtoint ptr %p to i32
67+
%r = sub i32 %q, -2147483644
68+
%s = inttoptr i32 %r to ptr
69+
%t = load i32, ptr %s
70+
ret i32 %t
71+
}
72+
73+
; CHECK-LABEL: dont_fold_non_nuw_sub_store:
74+
; CHECK: local.get 0
75+
; CHECK-NEXT: i32.const 2147483644
76+
; CHECK-NEXT: i32.add
77+
; CHECK-NEXT: i32.const 5
78+
; CHECK-NEXT: i32.store 0
79+
define void @dont_fold_non_nuw_sub_store(ptr %p) {
80+
%q = ptrtoint ptr %p to i32
81+
%r = sub i32 %q, -2147483644
82+
%s = inttoptr i32 %r to ptr
83+
store i32 5, ptr %s
84+
ret void
85+
}
86+
87+
; CHECK-LABEL: dont_fold_non_nuw_sub_load_2:
88+
; CHECK: i32.const mylabel
89+
; CHECK-NEXT: i32.const -4
90+
; CHECK-NEXT: i32.add
91+
; CHECK-NEXT: i32.load 0
92+
define i32 @dont_fold_non_nuw_sub_load_2() {
93+
%t = load i32, ptr inttoptr (i32 sub (i32 ptrtoint (ptr @mylabel to i32), i32 4) to ptr), align 4
94+
ret i32 %t
95+
}
96+
97+
; CHECK-LABEL: dont_fold_non_nuw_sub_store_2:
98+
; CHECK: i32.const mylabel
99+
; CHECK-NEXT: i32.const -4
100+
; CHECK-NEXT: i32.add
101+
; CHECK-NEXT: i32.const 5
102+
; CHECK-NEXT: i32.store 0
103+
define void @dont_fold_non_nuw_sub_store_2() {
104+
store i32 5, ptr inttoptr (i32 sub (i32 ptrtoint (ptr @mylabel to i32), i32 4) to ptr), align 4
105+
ret void
106+
}

‎llvm/test/CodeGen/WebAssembly/fast-isel-pr47040.ll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ target triple = "wasm32-unknown-unknown"
1414
define i32 @foo() {
1515
%stack_addr = alloca i32
1616
%stack_i = ptrtoint ptr %stack_addr to i32
17-
%added = add i32 %stack_i, undef
17+
%added = add nuw i32 %stack_i, undef
1818
%added_addr = inttoptr i32 %added to ptr
1919
%ret = load i32, ptr %added_addr
2020
ret i32 %ret

0 commit comments

Comments
 (0)