[PATCH] D34391: [RegisterCoalescer] Fix for SubRange join unreachable

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks for working on this! A bunch of nitpicks are below but overal the fix looks fine.



================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1228
+    //   vreg2<def> = COPY vreg1
+    //     ; vreg2:sub0 is actually undef but subrange exists in LiveRange for lane
+    // ==>
----------------
I would not describe `vreg2:sub0` as undef here, the `COPY` is a normal definition like any other.
It just happens that after coalescing we don't have a definition left because the copy was reading a partially undef value (but that effect will be visible after the `==>` arrow).


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1231
+    //   vreg2:sub1<def, read-undef> = LOAD CONSTANT 1
+    //     ; Correct but need to remove the subrange for sub0
+    if (NewIdx != 0 && DstIdx == 0 && DstInt.hasSubRanges()) {
----------------
This would be the place to mention that vreg2:sub0 is undef now and the subrange needs to be removed.


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1232
+    //     ; Correct but need to remove the subrange for sub0
+    if (NewIdx != 0 && DstIdx == 0 && DstInt.hasSubRanges()) {
+      // The affected subregister segments can be removed.
----------------
Why do you need `DstIdx == 0`, it seems to me that we need the fixup regardless of DstIdx.


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1238
+        if ((SR.LaneMask & DstMask).none()) {
+          DEBUG(dbgs() << "SubRange containing an undef tagged as def "
+                << PrintLaneMask(SR.LaneMask) << " : " << SR << "\n");
----------------
"undef tagged as def" is a strange description. How about: "Removing undefined subrange ..." as debug message?


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1241-1243
+          VNInfo *RmValNo = SR.Query(CurrIdx).valueOutOrDead();
+          if (RmValNo)
+            SR.removeValNo(RmValNo);
----------------
How about `if (VNInfo *RmValNo = getVNInfoAt(CurrIdx.getRegSlot()))` (It shouldn't matter here because NewMI should not write to that part of the register. But writing `getRegSlot()` feels more natural to check for liveranges going out of an instruction).


================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:1246
+      }
+    }
   } else if (NewMI.getOperand(0).getReg() != CopyDstReg) {
----------------
You should call `DstInt.removeEmptySubRanges()` after cleaning subranges.


https://reviews.llvm.org/D34391