Page MenuHomePhabricator

[WebAssembly] Handle unstackified TEE dest in ExplicitLocals
AbandonedPublic

Authored by aheejin on Jun 12 2020, 12:41 PM.

Details

Reviewers
dschuff
Summary

When created in RegStackify pass, tee has two destinations, where
operand 0 is stackified and operand 1 is not. But it is possible that
operand 0 is unstackified in fixUnwindMismatches function in CFGStackify
pass when a nested try-catch-end is introduced. In this case,
local.tee has lost its purpose, so we generate local.tee as planned
when op0 remains stackified, and generate local.set instead if op0 got
unstackified.

Related explanation in RegStackify: https://github.com/llvm/llvm-project/blob/339177d1da0c6f0cf181ba987836148acb526926/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp#L611-L630

For example,

  1. In RegStackify. (r0 is stackified and r1 is not)
r0, r1 = tee defreg
... use r0 ...
... use r1 ...
  1. In CFGStackify, r0 may get unstackified
  1. In ExplicitLocals,

3-a. If r0 remains stackified, use local.tee as planned:

r0 = local.tee N defreg
...
... use r0 ... (from stackified r0)
...
local.get N
... use r1 ... (from local.get N)

3-b. If r0 got unstackified, use local.set instead:

local.set N defrag
...
local.get N
... use r0 ... (from local.get N)
local.get N
... use r1 ... (from local.get N)

Diff Detail

Event Timeline

aheejin created this revision.Jun 12 2020, 12:41 PM
aheejin edited the summary of this revision. (Show Details)Jun 12 2020, 12:43 PM
aheejin edited the summary of this revision. (Show Details)Jun 12 2020, 12:45 PM
aheejin updated this revision to Diff 270497.Jun 12 2020, 12:46 PM
  • Fix typo in comment

(I'll use the term "de-stackify" as a verb to avoid confusion).
If fixUnwindMismatches de-stackifies the tee result, does that violate some kind of invariant? is it better to fix it up in fixUnwindMismatches?

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
277

does this mean that fixUnwindMismatches makes it unstackified or that it's already unstackified when fixUnwindMismatches runs?
edit:
I think it means the former, so maybe this could say "operand 0 becomes unstackified" instead. I guess this is all because "unstackified" could be a noun or a verb.... thanks English...

aheejin marked 2 inline comments as done.Jun 12 2020, 2:51 PM

(I'll use the term "de-stackify" as a verb to avoid confusion).

The relevant function name in CFGStackify is unstackifyVRegsUsedInSplitBB. Do you think this is better to be renamed to destackify- as well?

If fixUnwindMismatches de-stackifies the tee result, does that violate some kind of invariant? is it better to fix it up in fixUnwindMismatches?

That unstackifying behavior was added in D68218. This happens because, for example,

bb0:
  r0, r1 = tee defreg
  ...
  call @bar
  ...
  use stackified r0

This basic block can be split while we introduce inner try-catch for call @bar, in case its unwind destination mismatches. Then this becomes something like

bb0:
  r0, r1 = tee defreg
  ...
  try
  call @bar

bb1:
  catch
  ...

bb2:
  end_try
  ...
  use stackified r0??

We do register stackification only within a BB, so r0 cannot be stackified at this point. This is the reason why we unstackify it there. I don't think we can fix it in fixUnwindMismatches as long as we split BBs there.

Not sure what you mean by the invariant, but that means if one of tee's dest is stackified and the other is not, that violation can't be avoided because of the reason above. Where we check and deal with that invariant is ExplicitLocals, which this CL fixes.

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
277

Yeah it means former. Changed to 'becomes' as you suggested. And.. isn't it an adjective or a verb..?

aheejin updated this revision to Diff 270526.Jun 12 2020, 2:51 PM
aheejin marked an inline comment as done.
  • is -> gets / becomes

Maybe we could keep using "unstackify" as a verb and use "non-stackified" as an adjective. I like that better than "destackify" anyway.

Not sure what you mean by the invariant, but that means if one of tee's dest is stackified and the other is not, that violation can't be avoided because of the reason above. Where we check and deal with that invariant is ExplicitLocals, which this CL fixes.

Yeah, I would think that anytime there is a tee, one of its dest would be stackified, because that's the point of having a tee in the first place.

I get that we have to do the BB-splitting you mentioned. Would it be possible to replace the tee with a copy?
e.g.

bb0:
  r0 = copy defreg
  ... use defreg (instead of r1)
  try:
  call @bar

...
use use non-stackified r0

or if that's not possible, maybe even inserting a copy directly after the tee that immediately consumes the stackified r0 and sets a non-stackified reg would be ok, since the final result (after copies are eliminated) should probably be the same.

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
277

Yeah, I'm just saying it could be used as either, which is what can make things confusing.

aheejin abandoned this revision.Jun 15 2020, 9:38 AM

Closing in favor of D81851.