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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 3 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:327
+        continue;
+      }
+
----------------
aheejin wrote:
> 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? 
> ```
Nice idea! Done.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:939
+          ++SubsequentUse;
+        }
+
----------------
aheejin wrote:
> 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?
Can do.


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:132
+
+  # Reject nontrivial programs that have unused instructions
+  if has_unused_op(program):
----------------
aheejin wrote:
> 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
> ```
Ah, in that case the answer is similar. It's wasteful to test a program with an unused instruction in addition to an identical program without that instruction. The "nontrivial" qualification is because some programs of a single unused instruction were explicitly allowed by the `# Allow only multivalue single-op programs` check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72902