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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

The code is reasonable to me, but I am worried by the asserts not being true in general.
I think we should indeed remove SUBREG_TO_REG in the long run and maybe short term, let's turn the asserts into fatal_errors.
That way if this breaks in the wild we'll get reports of the related errors instead of miscompiles.

What do you think?



================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:1889
+      assert(!MRI->shouldTrackSubRegLiveness(DstReg) &&
+             "this should update subranges");
+      MachineInstrBuilder MIB(*MF, UseMI);
----------------
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.


================
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:
> 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?


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

https://reviews.llvm.org/D156345