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

Mon Mar 20 08:57:16 PDT 2023

tlively added a comment.

Oh oops, I had written a response but then I never clicked "submit." Sorry about that.

================
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:
> 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)
Sure, here's the "after" case I have in mind for when DefReg is not stackified. Both `TeeReg` and `Reg` have been replaced with `DefReg`, which will be accessed using `local.get` instructions inserted later.

//Removed: TeeReg, Reg = TEE DefReg//
INST ..., DefReg, ...
INST ..., DefReg, ...
INST ..., DefReg, ...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146084