[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.

Hi @arsenm ,

A maybe more robust temporary fix may be to apply the terminal rule (`applyTerminalRule`) only on `isCopy` instructions (right now it is on `isCopyLike`).

I could be wrong, but I wouldn't be surprised if this is the entry point of the problematic coalesced SUBREG_TO_REGs. And clearly without your fixes, the terminal rule is incorrect on SUBREG_TO_REG.

The reason I'm suggesting that is because I feel that properly fixing the liveness for SUBREG_TO_REG may take a while.
In particular the asserts you added are I believe possible to break.

Cheers,
-Quentin



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


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:2163
+  if (CP.getDstIdx()) {
+    assert(!IsSubregToReg && "can this happen?");
+    updateRegDefsUses(CP.getDstReg(), CP.getDstReg(), CP.getDstIdx(), false);
----------------
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 ...
```


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

https://reviews.llvm.org/D156345