[PATCH] D125515: [WebAssembly] Fix register use-def in FixIrreducibleControlFlow

[Alon Zakai via Phabricator via llvm-commits]

kripken accepted this revision.
kripken added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:533
+  }
+}
+
----------------
aheejin wrote:
> kripken wrote:
> > 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...)
> > 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_DEF`s because `ARGUMENT_` insts should be before all `IMPLICIT_DEF`s. 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_DEF`s, 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_DEF`s, 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.
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125515