[llvm] [WebAssembly] Don't fold non-nuw add/sub in FastISel (PR #111278)

Heejin Ahn via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 7 19:58:40 PDT 2024


================
@@ -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())
----------------
aheejin wrote:

I made this pattern of code `assert(false)` and ran emscripten `core0` tests with it, and the only case this pattern was generated is in asan tests.

This part in the normal ISel is hit a lot more because many other instructions, including `getelementptr`s, are turned into `ISD:ADD` in ISel. But this is in the normal ISel and there's no "bailout". (What I told you offline was actually hitting this cases in the normal ISel. I added `assert(false)` in both here and the newly added FastISel checks. So this one is not very relevant anyway.)
https://github.com/llvm/llvm-project/blob/65688274b14f5d0e7dabbbaf9f3fd135646ef1d1/llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp#L328-L332

So I guess, as @SingleAccretion pointed out, we don't need to worry too much about -O0 compilation time at least in C/C++. But this may regress -O0 compilation time in C# (or other languages). 

Currently our FastISel's `selectLoad` and `selectStore` only handles the cases where its address can be represented within a single `Address` class:
https://github.com/llvm/llvm-project/blob/db4874c2fc0b34e8959ff0ac455bff5f54fbd52b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp#L53-L110
And addressing modes are assumed to be either reg-based or frameindex-based. So we can handle cases like reg + label but not two different regs, which we need to handle here.

While it is not impossible to extend FastISel to handle the two-register cases, I guess that means we can handle a lot more cases that we are currently rejecting, which means even bigger overhaul. It's possible that we only handle the cases here, but we don't really know how frequent these cases compared to other cases we are already rejecting. Also given that this is a "Fast" ISel after all, I'm not sure whether a bigger extension of the pass is worthwhile.

How about landing this for now, because this is a correctness issue, and see if this is really regressing -O0 compilation times a lot? For that we'd appreciate feedback from C# devs (@SingleAccretion and @yowl).

https://github.com/llvm/llvm-project/pull/111278


More information about the llvm-commits mailing list