[PATCH] D72902: [WebAssembly] Fix RegStackify and ExplicitLocals to handle multivalue

[Heejin Ahn via Phabricator via llvm-commits]

aheejin accepted this revision.
aheejin added a comment.
This revision is now accepted and ready to land.

Nice work!



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:327
+        continue;
+      }
+
----------------
tlively wrote:
> aheejin wrote:
> > Why is this? Is this related to multivalue?
> > And Nit: We don't need `{}` for one-line ifs in LLVM :) I know it becomes a habit after working for a long time on Binaryen...
> The continue right after the `DidEraseMI = true` line used to continue the outer loop over the machine instructions. Since I added another level of looping, I needed to add a new continue referring to that loop here.
I don't think `IMPLICIT_DEF` will have ever have a multi-value def. Then can we check this and bail out right after the outer loop on starts? What I mean is
```
    for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E;) {  
      MachineInstr &MI = *I++;                                                   
      assert(!WebAssembly::isArgument(MI.getOpcode()));                          
                                                                                 
      if (MI.isDebugInstr() || MI.isLabel())                                     
        continue;

      // Maybe here? 
```


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:331
+  // stackified. But only one def can be stackified by moving the instruction,
+  // so it must be the first one.
+  //
----------------
tlively wrote:
> aheejin wrote:
> > Why is it impossible to place a def in a local if a subsequent def is stackified? This can't be possible?
> > ```
> > .functype pair_const () -> (i32, i64)
> > 
> > call pair_const # only i64 is stackified
> > use_i64
> > local.set 0
> > ...
> > local.get 0
> > use_i32
> > ```
> In principle, yes this is possible in some cases, but WebAssemblyExplicitLocals only knows how to place local.set instructions immediately after the instruction that defined the value being placed into a local. Generalizing this correctly would be a complex separate project. I changed this comment to point out that this is a limitation of ExplicitLocals.
Ah, you're right.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:928
+          // TODO: This single-use restriction could be relaxed by using tees
+          if (DefReg != UseReg || !MRI.hasOneUse(DefReg))
+            break;
----------------
tlively wrote:
> aheejin wrote:
> > If `DefReg` has multiple uses, can we create a new vreg to stackify that instead, as done in [[ https://github.com/llvm/llvm-project/blob/279fa8e0064e3d0bd1646b8efdb94045585dd924/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp#L498-L519 | here ]]? I'm not very sure how we can use tees here though.
> I'm not sure how that would work. Perhaps we can talk about it offline.
Oh, nevermind. I was confused and thinking about the case when `!MRI.hasOneDef(DefReg)` instead. 


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:939
+          ++SubsequentUse;
+        }
+
----------------
This does not currently seems to be able to handle a case that uses are spread across multiple consecutive instructions, such as
```
r0, r1 = inst
use r1
use r0
```
How about adding a TODO comment for this?


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:132
+
+  # Reject nontrivial programs that have unused instructions
+  if has_unused_op(program):
----------------
tlively wrote:
> aheejin wrote:
> > Just curious, I agree this kind of program will not be very interesting, but why would it be nontrivial? Does that mean it poses some difficulties?
> No, this filter exists only to reduce the number of programs. Defs with three uses are no more interesting than defs with two uses as far as stackification is concerned, so it's not worth emitting programs that have three uses of a def in addition to a program that has only two uses but is otherwise equivalent.
The location of the comments is messed up after updates ;( This comment was originally attached to the snippet below, and that's the reason why I asked about its 'nontriviality':
```
  # Reject nontrivial programs that have unused instructions
  if has_unused_op(program):
    return False
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72902/new/

https://reviews.llvm.org/D72902