This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add comments on local.tee transformation

Authored by aheejin on Mar 14 2023, 1:09 PM.



We have a good comment on TEE transformation in RegStackify:

And I think it can be helpful to have some more comments on how the
TEEs created in RegStackify are converted into LOCAL_TEEs.

Variable OldReg is changed to DefReg to be consistent with
RegStackify's comment.

Diff Detail

Event Timeline

aheejin created this revision.Mar 14 2023, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:09 PM
Herald added subscribers: pmatos, asb, wingo and 5 others. · View Herald Transcript
aheejin requested review of this revision.Mar 14 2023, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 1:09 PM
aheejin edited the summary of this revision. (Show Details)Mar 14 2023, 1:29 PM
aheejin edited the summary of this revision. (Show Details)
tlively accepted this revision.Mar 15 2023, 8:10 AM

LGTM % language tweaks!


I think the past tense makes it clearer that this refers to the "Before" state.


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?

This revision is now accepted and ready to land.Mar 15 2023, 8:10 AM
aheejin marked 3 inline comments as done.Mar 15 2023, 2:53 PM
aheejin added inline comments.

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: and

aheejin updated this revision to Diff 505633.Mar 15 2023, 2:53 PM

Address comments

aheejin added inline comments.Mar 17 2023, 2:30 PM

Ping 😀

aheejin added inline comments.Mar 17 2023, 8:16 PM

Will land this, given that this is more of a suggestion for a follow-up.

This revision was automatically updated to reflect the committed changes.

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


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, ...

aheejin added inline comments.Mar 28 2023, 1:29 AM

I think, as you said, if we delete the TEE and replace TeeReg and Reg with DefReg, we can save one instruction. The code may look like this:

if (!MFI.isVRegStackified(DefReg)) {
  Register TeeReg = MI.getOperand(0).getReg();
  Register Reg = MI.getOperand(1).getReg();
  MRI.replaceRegWith(TeeReg, DefReg);
  MRI.replaceRegWith(Reg, DefReg);
  Changed = true;

But I'm wondering if we can replace a register with another register like this give that we don't have SSA at this point in the pipeline. Also there can be many Regs down the line, so they can be in other BBs, at which point we cannot guarantee those Regs have a unique def. The weird thing is, we are doing this in another part of this pass, which I don't understand:

But that part of the code has existed for like 7 years now and hasn't caused any problems so far. Not sure why.

But anyway, for the TEE thing, given that it's not SSA anymore I'm not sure if I can replace a reg with another wholesale. But other that this it's hard to replace all Regs down the line, because we don't have SSA and thus no replaceAllUsesWith kind of thing.

But I think this, meaning DefReg unstackified by this point, currently happens in a very limited situation when we are making try-delegate and DefReg happens to be a stackified register in the boundary of newly splitted BBs so we unstackify it:

I think this case is rare enough, and not fixing this wouldn't make the code size meaningfully worse.
Also the weird thing is, this part of the code in ExplicitLocals has existed before that try-delegate handling, but I can't find anywhere we unstackify a register between RegStackify and here other that that. Not sure what was in Dan's mind 7(?) years ago at this point.