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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixIrreducibleControlFlow.cpp:533
+  }
+}
+
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp:108
+  // set.
+  MF.getProperties().set(MachineFunctionProperties::Property::TracksLiveness);
+
----------------
dschuff wrote:
> Should this go in the FixIrreducibleControlFlow pass? It's that pass that sets up the implicit defs correctly and not this one, right?
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_DEF`s in case of irreducible CFGs, we have to set this for every function, whether or not it is irreducible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125515