[PATCH] D156345: RegisterCoalescer: Add implicit-def of super register when coalescing SUBREG_TO_REG

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:1889
+      assert(!MRI->shouldTrackSubRegLiveness(DstReg) &&
+             "this should update subranges");
+      MachineInstrBuilder MIB(*MF, UseMI);
----------------
qcolombet wrote:
> arsenm wrote:
> > qcolombet wrote:
> > > It should be possible to produce a test case that hits this assertion today, no?
> > > In other words, won't we just miscompile in release mode in these case.
> > In general, you would have to go out of your way to enable subregister liveness.  Then I would expect a verifier error, and downstream uses to be about as broken as before.
> > 
> > Only AMDGPU, PowerPC, Hexagon RISCV enable it without a cl::opt, none of which use SUBREG_TO_REG. 
> > 
> > PowerPC is potentially problematic, as it does use SUBREG_TO_REG and enables subregister liveness by default (TIL)
> > 
> > 
> > 
> > PowerPC is potentially problematic, as it does use SUBREG_TO_REG and enables subregister liveness by default (TIL)
> 
> Did you get to the bottom of whether or not this is problematic?
> (Meaning the assert may break?)
> 
> If we're unsure maybe it would be best to report a fatal_error at this point.
So if I force on coalescing and verify coalescing, only one PPC test fails. Interestingly, the failure isn't even a PPC failure. It for some reason has a run line using the default triple, such that it compiles for x86.

The PPC usage of SUBREG_TO_REG also looks different, it seems to be using this for vectors and for f64 values (using the hint value 1, not 0). 

So my conclusion is that it's broken, but less likely to be observed. I think it's best to leave the subrange complexity for a separate patch, at least you hit the verifier error when it matters.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:2163
+  if (CP.getDstIdx()) {
+    assert(!IsSubregToReg && "can this happen?");
+    updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx(), false);
----------------
qcolombet wrote:
> qcolombet wrote:
> > Maybe it can happen if you have something like:
> > ```
> > %a = SUBREG_TO_REG ...
> > %b = IMPLICIT_DEF
> > %c = INSERT_SUBREG %b, %a, sub
> > ```
> > =>
> > ```
> > %a = SUBREG_TO_REG ...
> > %c.sub = COPY %a
> > ```
> > =>
> > ```
> > %c.sub = SUBREG_TO_REG ...
> > ```
> Ditto on the can this happen.
> Did you give a try to the example I gave?
If I force subregister liveness on ninja check does find the failures.

Also the baseline ninja check with verify-coalescing forced on isn't clean :(


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

https://reviews.llvm.org/D156345