[MachineVerifier] Relax checkLivenessAtDef regarding dead subreg defs

[bjope]

Summary

Consider an instruction that has multiple defs of the same
vreg, but defining different subregs:

%7.sub1:rc, dead %7.sub2:rc = inst

Calling checkLivenessAtDef for the live interval associated
with %7 incorrectly reported "live range continues after a
dead def". The live range for %7 has a dead def at the slot
index for "inst" even if the live range continues (given that
there are later uses of %7.sub1).

This patch adjusts MachineVerifier::checkLivenessAtDef
to allow dead subregister definitions, unless we are checking
a subrange (when tracking subregister liveness).

A limitation is that we do not detect the situation when the
live range continues past an instruction that defines the
full virtual register by multiple dead subreg defines.

I also removed some dead code related to physical register
in checkLivenessAtDef. Wwe only call that method for virtual
registers, so I added an assertion instead.

[bjope]

Almost forgot, I'll try to find a test case for an in-tree target!

[kparzysz]

In general, looking at a single instruction is not sufficient to tell whether a given register becomes dead or not, so I wouldn't worry about the limitation you mentioned. Cases that we can reliably handle are (sub)ranges corresponding to the parts of the register that is given in the operand (i.e. the entire register if there is no subregister).

lib/CodeGen/MachineVerifier.cpp

1572 ↗

(On Diff #165986)

I think this is also wrong. What's worse is that we seem to be tracking the liveness of whole registers here, not register:subreg. In a case with %0.sub1, dead %0.sub2 = ..., a subsequent use of %0.sub1 could cause verification failure. I'm not sure how much work it would take to fix this, it doesn't have to be in this patch.

[bjope]

Added a reproducer using hexagon as target, that way it was possible to show the difference between having subregister liveness or not.

[bjope]

lib/CodeGen/MachineVerifier.cpp

1572 ↗

(On Diff #165986)

Yes, it looks suspicious. Although, I got a feeling that it is out-of-scope for this patch.

Depending on how regsDead is used it might just make the verifier let bad things pass without any remarks. That could explain why we do not have problems with this.

llvm/trunk/lib/CodeGen/MachineVerifier.cpp

struct MachineVerifier {
     void verifyInlineAsm(const MachineInstr *MI);
 
     void checkLiveness(const MachineOperand *MO, unsigned MONum);
     void checkLivenessAtUse(const MachineOperand *MO, unsigned MONum,
                             SlotIndex UseIdx, const LiveRange &LR, unsigned VRegOrUnit,
                             LaneBitmask LaneMask = LaneBitmask::getNone());
     void checkLivenessAtDef(const MachineOperand *MO, unsigned MONum,
                             SlotIndex DefIdx, const LiveRange &LR, unsigned VRegOrUnit,
+                            bool SubRangeCheck = false,
                             LaneBitmask LaneMask = LaneBitmask::getNone());
 
     void markReachable(const MachineBasicBlock *MBB);
     void calcRegsPassed();
     void checkPHIOps(const MachineBasicBlock &MBB);
 
     void calcRegsRequired();
     void verifyLiveVariables();
if (MO->isKill() && !LRQ.isKill()) {
     if (LaneMask.any())
       report_context_lanemask(LaneMask);
     report_context(UseIdx);
   }
 }
 
 void MachineVerifier::checkLivenessAtDef(const MachineOperand *MO,
     unsigned MONum, SlotIndex DefIdx, const LiveRange &LR, unsigned VRegOrUnit,
-    LaneBitmask LaneMask) {
+    bool SubRangeCheck, LaneBitmask LaneMask) {
   if (const VNInfo *VNI = LR.getVNInfoAt(DefIdx)) {
     assert(VNI && "NULL valno is not allowed");
     if (VNI->def != DefIdx) {
       report("Inconsistent valno->def", MO, MONum);
       report_context_liverange(LR);
       report_context_vreg_regunit(VRegOrUnit);
       if (LaneMask.any())
         report_context_lanemask(LaneMask);
       report_context(*VNI);
       report_context(DefIdx);
     }
   } else {
     report("No live segment at def", MO, MONum);
     report_context_liverange(LR);
     report_context_vreg_regunit(VRegOrUnit);
     if (LaneMask.any())
       report_context_lanemask(LaneMask);
     report_context(DefIdx);
   }
   // Check that, if the dead def flag is present, LiveInts agree.
   if (MO->isDead()) {
     LiveQueryResult LRQ = LR.Query(DefIdx);
     if (!LRQ.isDeadDef()) {
-      // In case of physregs we can have a non-dead definition on another
-      // operand.
-      bool otherDef = false;
-      if (!TargetRegisterInfo::isVirtualRegister(VRegOrUnit)) {
-        const MachineInstr &MI = *MO->getParent();
-        for (const MachineOperand &MO : MI.operands()) {
-          if (!MO.isReg() || !MO.isDef() || MO.isDead())
-            continue;
-          unsigned Reg = MO.getReg();
-          for (MCRegUnitIterator Units(Reg, TRI); Units.isValid(); ++Units) {
-            if (*Units == VRegOrUnit) {
-              otherDef = true;
-              break;
-            }
-          }
-        }
-      }
-
-      if (!otherDef) {
+      assert(TargetRegisterInfo::isVirtualRegister(VRegOrUnit) &&
+             "Expecting a virtual register.");
+      // A dead subreg def only tells us that the specific subreg is dead. There
+      // could be other non-dead defs of other subregs, or we could have other
+      // parts of the register being live through the instruction. So unless we
+      // are checking liveness for a subrange it is ok for the live range to
+      // continue, given that we have a dead def of a subregister.
+      if (SubRangeCheck || MO->getSubReg() == 0) {
         report("Live range continues after dead def flag", MO, MONum);
         report_context_liverange(LR);
         report_context_vreg_regunit(VRegOrUnit);
         if (LaneMask.any())
           report_context_lanemask(LaneMask);
       }
     }
   }
if (LiveInts && !LiveInts->isNotInMIMap(*MI)) {
           if (LI.hasSubRanges()) {
             unsigned SubRegIdx = MO->getSubReg();
             LaneBitmask MOMask = SubRegIdx != 0
               ? TRI->getSubRegIndexLaneMask(SubRegIdx)
               : MRI->getMaxLaneMaskForVReg(Reg);
             for (const LiveInterval::SubRange &SR : LI.subranges()) {
               if ((SR.LaneMask & MOMask).none())
                 continue;
-              checkLivenessAtDef(MO, MONum, DefIdx, SR, Reg, SR.LaneMask);
+              checkLivenessAtDef(MO, MONum, DefIdx, SR, Reg, true, SR.LaneMask);
             }
           }
         } else {
           report("Virtual register has no Live interval", MO, MONum);
         }
       }
     }
   }

llvm/trunk/test/CodeGen/Hexagon/verify-liveness-at-def.mir

+# Using a trick to run simple-register-coalescing twice, that way
+# liveintervals should be preserved while running the machine verifier.
+#
+# RUN: not llc -o - %s -march=hexagon -hexagon-subreg-liveness=false -run-pass simple-register-coalescing -verify-machineinstrs -run-pass simple-register-coalescing 2>&1 | FileCheck -check-prefix=CHECK-NOSUB %s
+# RUN: not llc -o - %s -march=hexagon -hexagon-subreg-liveness=true -run-pass simple-register-coalescing -verify-machineinstrs -run-pass simple-register-coalescing 2>&1 | FileCheck -check-prefix=CHECK-SUB %s
+
+---
+name: test_pass
+tracksRegLiveness: true
+body: |
+  bb.0:
+    A2_nop implicit-def %0:doubleregs
+    A2_nop implicit-def dead %0.isub_lo, implicit-def %0.isub_hi, implicit %0
+    A2_nop implicit %0.isub_hi
+...
+
+---
+name: test_fail
+tracksRegLiveness: true
+body: |
+  bb.0:
+    A2_nop implicit-def %0:doubleregs
+    A2_nop implicit-def dead %0.isub_lo, implicit-def %0.isub_hi, implicit %0
+    A2_nop implicit %0.isub_lo
+
+    A2_nop implicit-def %1:doubleregs
+    A2_nop implicit-def dead %1.isub_lo, implicit-def dead %1.isub_hi, implicit %1
+    A2_nop implicit %1
+
+    A2_nop implicit-def dead %2:doubleregs
+    A2_nop implicit %2
+
+...
+
+###############################################################################
+# We are expecting four "Bad machine code" when subregister liveness is used.
+#
+# CHECK-SUB-NOT: Bad machine code
+#
+# CHECK-SUB: Bad machine code: Live range continues after dead def flag
+# CHECK_SUB-NEXT: function:    test_fail
+# CHECK-SUB:      v. register: %0
+# CHECK-SUB:      lanemask:    00000002
+#
+# CHECK-SUB-NOT: Bad machine code
+#
+# CHECK-SUB: Bad machine code: Live range continues after dead def flag
+# CHECK-SUB-NEXT: function:    test_fail
+# CHECK-SUB:      v. register: %1
+# CHECK-SUB:      lanemask:    00000002
+#
+# CHECK-SUB-NOT: Bad machine code
+#
+# CHECK-SUB: Bad machine code: Live range continues after dead def flag
+# CHECK-SUB-NEXT: function:    test_fail
+# CHECK-SUB:      v. register: %1
+# CHECK-SUB:      lanemask:    00000001
+#
+# CHECK-SUB: Bad machine code: Live range continues after dead def flag
+# CHECK-SUB-NEXT: function:    test_fail
+# CHECK:          v. register: %2
+#
+# CHECK-SUB-NOT: Bad machine code
+
+###############################################################################
+# Without subregister liveness we only detect one of the failing scenarios.
+#
+# CHECK-NOSUB-NOT: Bad machine code
+#
+# CHECK-NOSUB: Bad machine code: Live range continues after dead def flag
+# CHECK-NOSUB-NEXT: function:    test_fail
+# CHECK:            v. register: %2
+#
+# CHECK-NOSUB-NOT: Bad machine code