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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice work! Sorry for the delayed reply. I like the test generation script and the autogenerated test case, and given the nature of the tests, I don't think there will be many changes to the generated results. With larger number of limits we can maybe use the script like a fuzzer or something too.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:327
+        continue;
+      }
+
----------------
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...


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:839
       while (!TreeWalker.done()) {
-        MachineOperand &Op = TreeWalker.pop();
+        MachineOperand &Use = TreeWalker.pop();
 
----------------
tlively wrote:
> I am thinking of splitting the renaming and re-typing of these variables into a separate NFC patch. Thoughts?
Sounds good. But if that creates extra work, I'm OK with as is.


================
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.
+  //
----------------
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
```


================
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;
----------------
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.


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:10
+The output of this script is meant to be used in conjunction with
+update_llc_test_checks.py.
+
----------------
What's the command to generate test results written in multivalue-stackify.ll? Do we need any options? If so, it'd be good to note that too.


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:20
+generated by this script. Once that is done, exhaustive testing can be done by
+making `is_interesting` return True.
+"""
----------------
What do you mean by the abstract interpreter? An interpreter for ll file, or .s / wast file, or both? I don't fully understand what kind of interpreter you are planning to make and how you are gonna use it. And do you mean optimizing RegStackify further by 'embarking on'?


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:30
+MAX_DEFS = 3
+MAX_USES = 2
+
----------------
It looks `MAX_OPS` and `MAX_DEFS` pertain to a whole program whereas `MAX_USES` is for a single instruction, right? How about renaming them to make it clear?


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:40
+
+def possible_ops(program):
+  program_defs = get_num_defs(program)
----------------
Nit: This returns an op, but the name sounds it returns multiple of them.. can we rename it to make it clearer?


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:132
+
+  # Reject nontrivial programs that have unused instructions
+  if has_unused_op(program):
----------------
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?


================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue.ll:11
 %pair = type { i32, i64 }
-%packed_pair = type <{ i32, i64 }>
 
----------------
We don't test packed pairs anymore? Is that because they don't have any differences in results from normal pairs? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72902