This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix register use-def in FixIrreducibleControlFlow
ClosedPublic

Authored by aheejin on May 12 2022, 8:13 PM.

Details

Summary

FixIrreducibleControlFlow pass adds dispatch blocks with a br_table
that has multiple predecessors and successors, because it serves as
something like a traffic hub for BBs. As a result of this, there can be
register uses that are not dominated by a def in every path from the
entry block. For example, suppose register %a is defined in BB1 and used
in BB2, and there is a single path from BB1 and BB2:

BB1 -> ... -> BB2

After FixIrreducibleControlFlow runs, there can be a dispatch block
between these two BBs:

BB1 -> ... -> Dispatch -> ... -> BB2

And this dispatch block has multiple predecessors, now
there is a path to BB2 that does not first visit BB1, and in that path
%a is not dominated by a def anymore.

To fix this problem, we have been adding IMPLICIT_DEFs to all
registers in PrepareForLiveInternals pass, and then remove unnecessary
ones in OptimizeLiveIntervals pass after computing LiveIntervals. But
FixIrreducibleControlFlow pass itself ends up violating register use-def
relationship, resulting in invalid code. This was OK so far because
MIR verifier apparently didn't check this in validation. But @arsenm
fixed this and it caught this bug in validation
(https://github.com/llvm/llvm-project/issues/55249).

This CL moves the IMPLICIT_DEF adding routine from
PrepareForLiveInternals to FixIrreducibleControlFlow. We only run it
when FixIrreducibleControlFlow changes the code. And then
PrepareForLiveInternals doesn't do anything other than setting
TracksLiveness property, which is a prerequisite for running
LiveIntervals analysis, which is required by the next pass
OptimizeLiveIntervals.

But in our backend we don't seem to do anything that invalidates this up
until OptimizeLiveIntervals, and I'm not sure why we are calling
invalidateLiveness in ReplacePhysRegs pass, because what that pass
does is to replace physical registers with virtual ones 1-to-1. I
deleted the invalidateLiveness call there and we don't need to set
that flag explicitly, which obviates all the need for
PrepareForLiveInternals.

(By the way, This 'Liveness' here is different from LiveIntervals
analysis. Setting this only means BBs' live-in info is correct, all uses
are dominated by defs, kill flag is conservatively correct, which
means if there is a kill flag set it should be the last use. See
https://github.com/llvm/llvm-project/blob/2a0837aab1489c88efb03784e34c4dc9f2e28302/llvm/include/llvm/CodeGen/MachineFunction.h#L125-L134
for details.)

So this CL removes PrepareForLiveInternals pass altogether. Something
similar to this was attempted by D56091 long ago but that came short of
actually removing the pass, and I couldn't land it because
FixIrreducibleControlFlow violated use-def relationship, which this CL
fixes.

This doesn't change output in any meaningful way. All test changes
except irreducible-cfg.mir are register numbering.

Also this will likely to reduce compilation time, because we have been
adding IMPLICIT_DEF for all registers every time -O2 is given, but
now we do that only when there is irreducible control flow, which is
rare.

Fixes https://github.com/llvm/llvm-project/issues/55249.

Diff Detail

Event Timeline

aheejin created this revision.May 12 2022, 8:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 8:13 PM
Herald added subscribers: pmatos, asb, wingo and 6 others. · View Herald Transcript
aheejin requested review of this revision.May 12 2022, 8:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2022, 8:13 PM
aheejin added inline comments.May 12 2022, 8:17 PM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
75–77

I'm not sure why these were here. This pass replaces physical registers with virtual ones, and this is one-to-one relationship so I think this pass doesn't change anything in liveness or SSA.

aheejin updated this revision to Diff 429127.May 12 2022, 8:18 PM

Remove a stray line

aheejin edited the summary of this revision. (Show Details)May 12 2022, 8:32 PM
aheejin edited the summary of this revision. (Show Details)May 13 2022, 12:59 AM

This looks great! I guess the test for this will be when the fix for the MIR verifier lands, and it will start throwing errors if this somehow breaks, right?

llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

Should this go in the FixIrreducibleControlFlow pass? It's that pass that sets up the implicit defs correctly and not this one, right?

kripken added inline comments.May 13 2022, 1:01 PM
llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp
533

I'm not that familiar with LLVM, but I am guessing that this code will add an implicit def for each register? That is, this is a change over all the registers used in the function, as opposed to adding definitions in specific blocks for specific registers, and at the top of the function it writes a 0 to all of them?

If so, then I wonder if this has performance implications? Specifically I worry about a very large function that has a tiny irreducible part nested in it. Would this increase live ranges in the entire function?

Even if it does, maybe other passes are smart enough to not be limited by it? fwiw, after LLVM, in Binaryen this should not be a problem (adding more local.sets at the top of the function has no effect - we already assume all wasm locals have an initial set of 0).

If this is a potential problem in LLVM, then I think the minimal thing to do here is to define all registers that pass through the dispatch block, in the dispatch block.

(Apologies if I've misunderstood the code here...)

aheejin added inline comments.May 13 2022, 3:42 PM
llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp
533

I'm not that familiar with LLVM, but I am guessing that this code will add an implicit def for each register? That is, this is a change over all the registers used in the function, as opposed to adding definitions in specific blocks for specific registers, and at the top of the function it writes a 0 to all of them?

If so, then I wonder if this has performance implications? Specifically I worry about a very large function that has a tiny irreducible part nested in it. Would this increase live ranges in the entire function?

The register number changes in our test cases stemmed from moving ARGUMENT_ instructions and not from IMPLICIT_DEF itself. We do this after adding IMPLICIT_DEFs because ARGUMENT_ insts should be before all IMPLICIT_DEFs. While doing so the order of ARGUMENT_ instructions can change and it looks it can result in register number differences later.

// Move ARGUMENT_* instructions to the top of the entry block, so that their
// liveness reflects the fact that these really are live-in values.
for (MachineInstr &MI : llvm::make_early_inc_range(Entry)) {
  if (WebAssembly::isArgument(MI.getOpcode())) {
    MI.removeFromParent();
    Entry.insert(Entry.begin(), &MI);
  }
}

I don't think number changes will affect performance in any meaningful way, especially in the optimized build. OptimizeLiveIntervals pass removes all unnecesary IMPLICIT_DEFs, so the live ranges will not be extended: https://github.com/llvm/llvm-project/blob/4205f4aba4aff74fa7681c3f991ef5fdaed48d35/llvm/lib/Target/WebAssembly/WebAssemblyOptimizeLiveIntervals.cpp#L105-L116

Even if it does, maybe other passes are smart enough to not be limited by it? fwiw, after LLVM, in Binaryen this should not be a problem (adding more local.sets at the top of the function has no effect - we already assume all wasm locals have an initial set of 0).

We have OptimizeLiveIntervals pass that removes unnecessary IMPLICIT_DEFs, and passes like RegColoring tries to reduce the number of registers used.

If this is a potential problem in LLVM, then I think the minimal thing to do here is to define all registers that pass through the dispatch block, in the dispatch block.

This can rewrite existing values, which can lead to incorrect code, I think? Also I can't think of any benefits, and we may need to do this multiple times if there are more than one dispatch block.

llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

It can go in any pass before OptimizeLiveIntervals pass. We can put that in FixIrreducibleCFG, but I'm not sure if that pass is any special, because every pass preserves correct register def-use relationship (except for FixIrreducibleControlFlow, until now) and IMPLICIT_DEF is just a way of preserving it in that pass. Also, note that while we only add IMPLICIT_DEFs in case of irreducible CFGs, we have to set this for every function, whether or not it is irreducible.

kripken accepted this revision.May 13 2022, 4:38 PM
kripken added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp
533

Sounds good, thanks for the details!

This can rewrite existing values, which can lead to incorrect code, I think? Also I can't think of any benefits, and we may need to do this multiple times if there are more than one dispatch block.

Hmm, yeah, it's definitely not as simple as I was thinking...

This revision is now accepted and ready to land.May 13 2022, 4:38 PM
arsenm added inline comments.May 16 2022, 3:17 PM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

This should move into the pass's getSetProperties/getClearedProperties

aheejin updated this revision to Diff 429904.May 16 2022, 5:05 PM

Add getRequiredProperties call to OptimizeLiveIntervals

llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

Thanks for the info! I wasn't aware of these APIs. But come to think about it, I don't think we even need that, because I deleted invalidateLiveness call in this pass above. I'm not sure why it was there in the first place, because we don't do anything to invalidate liveness here; what this pass does is to replace physical registers with virtual ones 1-to-1, which has nothing to do with liveness.

Also I removed invalidateLiveness call in FixIrreducibleControlFlow pass; we now add IMPLICIT_DEFs there so it should be fine.

So in short we don't invalidate liveness up until CFGSort/CFGStackify, which is later than this and near the end of the pipeline, and the thing we invalidate there is kill flag and not use-def relationship. So I don't think we need to set this anywhere.

But to make things clear, I'll instead add this to WebAssemblyOptimizeLiveIntervals, the next pass, to make sure what this pass requires.

MachineFunctionProperties getRequiredProperties() const override {
  return MachineFunctionProperties().set(
      MachineFunctionProperties::Property::TracksLiveness);
}
aheejin edited the summary of this revision. (Show Details)May 16 2022, 6:04 PM
dschuff accepted this revision.May 17 2022, 9:55 AM
dschuff added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

so you're saying, none of these passes now either set or clear the property, they just preserve it; so there's no need to mark it explicitly (other than ReplacePhysRegs where we actually remove the physregs). I think that makes sense.

aheejin updated this revision to Diff 430125.May 17 2022, 10:50 AM

Remove getSetProperties

aheejin added inline comments.May 17 2022, 10:52 AM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

Oh, sorry, that's not what I meant. I made the confusion because I also added getSetProperties.. That was an incorrect snippet I added while experimenting.

So you're saying, none of these passes now either set or clear the property, they just preserve it;

This is correct.

so there's no need to mark it explicitly (other than ReplacePhysRegs where we actually remove the physregs).

I believe even ReplacePhysRegs doesn't need to remove this property. Note that I deleted the call to invalidateLiveness on line 77 in this file. So we don't need to set the property again here, meaning we don't need go call getSetProperties. This was added in the diff, which made the confusion. I deleted the call.

dschuff added inline comments.May 17 2022, 11:00 AM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

I believe even ReplacePhysRegs doesn't need to remove this property. Note that I deleted the call to invalidateLiveness on line 77 in this file. So we don't need to set the property again here, meaning we don't need go call getSetProperties. This was added in the diff, which made the confusion. I deleted the call.

Sorry I actually was seeing that in the last patch set it *added* the property. I assumed that was because it was only the physregs that may have been violating it. But yes it still makes sense that none of the passes needs to set or clear the property, and mark OptimizeLiveIntervals as requiring it.

aheejin added inline comments.May 17 2022, 11:16 AM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

Yeah that was confusing. So these register liveins only apply to physical registers, so after we remove all physical registers here, we don't violate livein requirements for sure. But were we violating the requirements before, even if we were using physical registers before this pass? Those physical registers are sp and fp, and they are added in PrologEpilogInserter or somewhere, and I think that pass should take care of preserving that. And a brief 1 min look on that pass looks like it does. Even if it doesn't, it's surely not that we should remove the property here; we should have rather set it here. (But as I said, we didn't remove it anywhere, so we don't need to set it again)

dschuff added inline comments.May 17 2022, 11:24 AM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

yeah, makes sense now. thanks for checking sp and fp. This patchset LGTM.

arsenm added inline comments.May 17 2022, 1:53 PM
llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp
115

sp and fp are presumably reserved registers, and thus not added to liveins lists or have tracked liveness anyway

This revision was landed with ongoing or failed builds.May 19 2022, 11:14 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/WebAssembly/WebAssemblyOptimizeLiveIntervals.cpp