[PATCH] D16693: Make sure all subranges are define during re-materialization of constants in RegCoalescer

[Matthias Braun via llvm-commits]

MatzeB added a comment.

Thanks for working on this.

The MachineVerifier enhancement could be split into a separate commit and is good to go with the issues below addressed. It could probably be extended to handle use operands as well but that can be done in a separate commit.

I will need more time to review the register coalescer code. I'm not convinced yet that the unrelated subregisters that lack the liveness segments are always free, if they are not then we just cannot do the rematerialisation.


================
Comment at: lib/CodeGen/MachineVerifier.cpp:1721
@@ +1720,3 @@
+  if (LI.hasSubRanges()) {
+    for (auto DI = MRI->def_begin(Reg), DE = MachineRegisterInfo::def_end();
+         DI != DE; ++DI) {
----------------
- MRI->def_end() is shorter and looks more uniform. 
- I'd welcome a `MachineRegisterInfo::def_iterator` instead of the auto because a reader cannot easily deduce the type here.

Alternatively you could use this:

```
for (const MachineOperand &MO : make_range(MRI->def_begin(Reg), MRI->def_end())) { ... }
```

================
Comment at: lib/CodeGen/MachineVerifier.cpp:1727
@@ +1726,3 @@
+      if (MO.getSubReg() == 0) {
+        unsigned LiveMask = 0;
+        for (auto &SR : LI.subranges()) {
----------------
`LaneBitmask LiveMask = 0;`

================
Comment at: lib/CodeGen/MachineVerifier.cpp:1728
@@ +1727,3 @@
+        unsigned LiveMask = 0;
+        for (auto &SR : LI.subranges()) {
+          SlotIndex DefSlot =
----------------
`LiveInterval::SubRange` instead of auto is friendlier for the reader.

================
Comment at: lib/CodeGen/MachineVerifier.cpp:1734-1737
@@ +1733,6 @@
+        }
+        // If any lane didn't have a proper definition report.
+        if (LiveMask != MaxMask)
+          report("Lane mask defined but no sub range reports its definition",
+                 MF);
+      }
----------------
I think you can make this test more powerful if you leave out the if (MO.getSubReg() == 0) above and instead do something like this here:

```
LaneBitmask MOMask = TRI->getSubRegIndexLaneMask(MO.getSubReg());
if ( (LiveMask & MOMask) != MOMask) { report(...); }
```

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1016
@@ +1015,3 @@
+    // subranges missing the definition.
+    if (MRI->shouldTrackSubRegLiveness(DstReg)) {
+      LiveInterval &DstInt = LIS->getInterval(DstReg);
----------------
I think you can leave out this check, the DstInt.hasSubRanges() can only happen if this is enabled anyway.

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1020
@@ +1019,3 @@
+        SlotIndex CurrIdx = LIS->getInstructionIndex(NewMI);
+        unsigned MaxMask = MRI->getMaxLaneMaskForVReg(DstReg);
+        for (auto &SR : DstInt.subranges()) {
----------------
`LaneBitmask MaxMask = ...`


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1021
@@ +1020,3 @@
+        unsigned MaxMask = MRI->getMaxLaneMaskForVReg(DstReg);
+        for (auto &SR : DstInt.subranges()) {
+          if (!SR.liveAt(CurrIdx.getRegSlot())) {
----------------
see above.

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1022-1024
@@ +1021,5 @@
+        for (auto &SR : DstInt.subranges()) {
+          if (!SR.liveAt(CurrIdx.getRegSlot())) {
+            SR.createDeadDef(CurrIdx.getRegSlot(), LIS->getVNInfoAllocator());
+          }
+          MaxMask &= ~SR.LaneMask;
----------------
Do not use {} here (llvm coding convention)

================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1030
@@ +1029,3 @@
+              DstInt.createSubRange(LIS->getVNInfoAllocator(), MaxMask);
+          SR->createDeadDef(CurrIdx.getRegSlot(), LIS->getVNInfoAllocator());
+        }
----------------
I think this should be
```
SlotIndex DefIndex = CurrIdx.getRegSlot(NewMI->getOperand(0).isEarlyClobber());
SR->createDeadDef(DefIndex, ...);
```

 (even though I don't think any of the GPU targets with subregisters use early clobbers).


Repository:
  rL LLVM

http://reviews.llvm.org/D16693