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

Thu Feb 13 16:13:04 PST 2020

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

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

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

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

================
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.
+
----------------
aheejin wrote:
> 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.
Added a code block with explicit commands.

================
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.
+"""
----------------
aheejin wrote:
> 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'?
Actually, we need two abstract interpreters. One would interpret the LLVM IR or the internal data structure from which it is generated and one would interpret the emitted wasm instructions. The interpreters are abstract because they do not operate over concrete i32 values, but rather abstract representations of the program fragment semantics. We would would want to assert that each LLVM program and its corresponding wasm program have identical semantics.

By "embarking on a rewrite" I just mean starting a rewrite of RegStackify, with the additional connotation that such a rewrite would be an interesting and long journey.

================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:30
+MAX_DEFS = 3
+MAX_USES = 2
+
----------------
aheejin wrote:
> 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?
Sounds good. Done.

================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue-stackify.py:40
+
+def possible_ops(program):
+  program_defs = get_num_defs(program)
----------------
aheejin wrote:
> Nit: This returns an op, but the name sounds it returns multiple of them.. can we rename it to make it clearer?
This function generates multiple ops, but one at a time. I think it is clearer to have a plural name here because the usage is `for op in possible_ops(...)`.

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

================
Comment at: llvm/test/CodeGen/WebAssembly/multivalue.ll:11
 %pair = type { i32, i64 }
-%packed_pair = type <{ i32, i64 }>

----------------
aheejin wrote:
> We don't test packed pairs anymore? Is that because they don't have any differences in results from normal pairs? 
Yeah, there was literally no difference in the backend so it didn't seem useful to test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72902