[PATCH] D146084: [WebAssembly] Add comments on local.tee transformation

Fri Mar 17 20:16:13 PDT 2023

aheejin added inline comments.

================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:289-290
+      // - After (when DefReg is not stackified):
+      // NewReg = LOCAL_GET LocalId1
+      // TeeReg = LOCAL_TEE LocalId2, NewReg
+      // INST ..., TeeReg, ...
----------------
aheejin wrote:
> aheejin wrote:
> > tlively wrote:
> > > I know you're just documenting the existing behavior, but wouldn't it be better to remove the tee and map both `TeeReg` and `Reg` to `LocalId1` without introducing `NewReg` or `LocalId2` at all?
> > Not sure what you are suggesting. Can you show what the code will look like (in terms of the existing `INST`, `TeeReg`, `Reg`, and such)?
> > 
> > `TeeReg` is stackified (and not mapped to a local) in both cwhen `DefReg` was stackified and not stackified). Are you suggesting `TeeReg` to be mapped to a local here? I don't think something can be mapped to a local and stackified at the same time.
> > 
> > When a reg is mapped to a local, that reg doesn't appear in the MIR code anymore. The reason `Reg` appears in `INST ..., Reg, ...` is because we are processing instructions sequentially and haven't processed that instruction yet. When we process each instruction, we replace each mapped reg to a local.
> > 
> > On the other hand, stackified registers (such as `TeeReg` or `NewReg`) will still appear in MIR code, but they in effect represent stack pushes and pops, and instructions will be stripped of them in `MCInstLower` or somewhere. So we create a new register whenever we stackify a register so that it won't clash with existing ones.
> > (Examples elsewhere:
> > https://github.com/llvm/llvm-project/blob/aba4e4d6c1c40ea7b8ba793627b62dc83a47a1d0/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L308 and
> > https://github.com/llvm/llvm-project/blob/aba4e4d6c1c40ea7b8ba793627b62dc83a47a1d0/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L380)
> Ping 😀
Will land this, given that this is more of a suggestion for a follow-up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146084