diff options
author | Heejin Ahn <aheejin@gmail.com> | 2024-10-09 14:31:16 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-10-09 14:31:16 -0700 |
commit | 115cb402d8ed91f94d22afcc4c2c9ed9def53cc7 (patch) | |
tree | f115349986cb72c1bb46598523d3db807bfc0ffa /llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp | |
parent | 853c43d04a378c379e49db552e856f02a5ad9216 (diff) | |
download | llvm-115cb402d8ed91f94d22afcc4c2c9ed9def53cc7.zip llvm-115cb402d8ed91f94d22afcc4c2c9ed9def53cc7.tar.gz llvm-115cb402d8ed91f94d22afcc4c2c9ed9def53cc7.tar.bz2 |
[WebAssembly] Don't fold non-nuw add/sub in FastISel (#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.
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.
Diffstat (limited to 'llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp')
-rw-r--r-- | llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp | 12 |
1 files changed, 12 insertions, 0 deletions
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp index 317c646..7c90fff 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()) + break; + // 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()) + break; + // Subs of constants are common and easy enough. const Value *LHS = U->getOperand(0); const Value *RHS = U->getOperand(1); |