[MachineVerifier] Verify liveins for live-through segments

[foad]

FIXME: CodeGen/X86/2012-01-10-UndefExceptionEdge.ll fails

Fixed by D149954.

[arsenm]

Can you add a unit test for this?

[foad]

In D149947#4329844, @arsenm wrote:

Can you add a unit test for this?

I'm not sure. Do we have any unit tests for specific MachineVerifier failures? I couldn't see any.

[arsenm]

In D149947#4341662, @foad wrote:

In D149947#4329844, @arsenm wrote:

Can you add a unit test for this?

I'm not sure. Do we have any unit tests for specific MachineVerifier failures? I couldn't see any.

I was thinking of unittests/MI/LiveIntervalTest.cpp. You can't really write a plain MIR test to produce broken live intervals

[foad]

Add a unit test.

[foad]

Add a comment.

llvm/lib/CodeGen/MachineVerifier.cpp

const MachineBasicBlock *EndMBB =
     LiveInts->getMBBFromIndex(S.end.getPrevSlot());
   if (!EndMBB) {
     report("Bad end of live segment, no basic block", MF);
     report_context(LR, Reg, LaneMask);
     report_context(S);
     return;
   }
 
-  // No more checks for live-out segments.
-  if (S.end == LiveInts->getMBBEndIdx(EndMBB))
-    return;
-
+  // Checks for non-live-out segments.
+  if (S.end != LiveInts->getMBBEndIdx(EndMBB)) {
     // RegUnit intervals are allowed dead phis.
     if (!Reg.isVirtual() && VNI->isPHIDef() && S.start == VNI->def &&
         S.end == VNI->def.getDeadSlot())
       return;
 
     // The live segment is ending inside EndMBB
     const MachineInstr *MI =
         LiveInts->getInstructionFromIndex(S.end.getPrevSlot());
     if (!MI) {
       report("Live segment doesn't end at a valid instruction", EndMBB);
       report_context(LR, Reg, LaneMask);
       report_context(S);
       return;
     }
 
     // The block slot must refer to a basic block boundary.
     if (S.end.isBlock()) {
       report("Live segment ends at B slot of an instruction", EndMBB);
       report_context(LR, Reg, LaneMask);
       report_context(S);
     }
 
     if (S.end.isDead()) {
       // Segment ends on the dead slot.
       // That means there must be a dead def.
       if (!SlotIndex::isSameInstr(S.start, S.end)) {
         report("Live segment ending at dead slot spans instructions", EndMBB);
         report_context(LR, Reg, LaneMask);
         report_context(S);
       }
     }
 
     // After tied operands are rewritten, a live segment can only end at an
     // early-clobber slot if it is being redefined by an early-clobber def.
-  // TODO: Before tied operands are rewritten, a live segment can only end at an
-  // early-clobber slot if the last use is tied to an early-clobber def.
+    // TODO: Before tied operands are rewritten, a live segment can only end at
+    // an early-clobber slot if the last use is tied to an early-clobber def.
     if (MF->getProperties().hasProperty(
             MachineFunctionProperties::Property::TiedOpsRewritten) &&
         S.end.isEarlyClobber()) {
-    if (I+1 == LR.end() || (I+1)->start != S.end) {
+      if (I + 1 == LR.end() || (I + 1)->start != S.end) {
         report("Live segment ending at early clobber slot must be "
-             "redefined by an EC def in the same instruction", EndMBB);
+               "redefined by an EC def in the same instruction",
+               EndMBB);
         report_context(LR, Reg, LaneMask);
         report_context(S);
       }
     }
 
     // The following checks only apply to virtual registers. Physreg liveness
     // is too weird to check.
     if (Reg.isVirtual()) {
       // A live segment can end with either a redefinition, a kill flag on a
       // use, or a dead flag on a def.
       bool hasRead = false;
       bool hasSubRegDef = false;
       bool hasDeadDef = false;
       for (ConstMIBundleOperands MOI(*MI); MOI.isValid(); ++MOI) {
         if (!MOI->isReg() || MOI->getReg() != Reg)
           continue;
         unsigned Sub = MOI->getSubReg();
-      LaneBitmask SLM = Sub != 0 ? TRI->getSubRegIndexLaneMask(Sub)
-                                 : LaneBitmask::getAll();
+        LaneBitmask SLM =
+            Sub != 0 ? TRI->getSubRegIndexLaneMask(Sub) : LaneBitmask::getAll();
         if (MOI->isDef()) {
           if (Sub != 0) {
             hasSubRegDef = true;
             // An operand %0:sub0 reads %0:sub1..n. Invert the lane
             // mask for subregister defs. Read-undef defs will be handled by
             // readsReg below.
             SLM = ~SLM;
           }
           if (MOI->isDead())
             hasDeadDef = true;
         }
         if (LaneMask.any() && (LaneMask & SLM).none())
           continue;
         if (MOI->readsReg())
           hasRead = true;
       }
       if (S.end.isDead()) {
         // Make sure that the corresponding machine operand for a "dead" live
         // range has the dead flag. We cannot perform this check for subregister
         // liveranges as partially dead values are allowed.
         if (LaneMask.none() && !hasDeadDef) {
-        report("Instruction ending live segment on dead slot has no dead flag",
-               MI);
+          report(
+              "Instruction ending live segment on dead slot has no dead flag",
           report_context(LR, Reg, LaneMask);
           report_context(S);
         }
       } else {
         if (!hasRead) {
           // When tracking subregister liveness, the main range must start new
           // values on partial register writes, even if there is no read.
           if (!MRI->shouldTrackSubRegLiveness(Reg) || LaneMask.any() ||
               !hasSubRegDef) {
             report("Instruction ending live segment doesn't read the register",
                    MI);
             report_context(LR, Reg, LaneMask);
             report_context(S);
           }
         }
       }
     }
+  }
 
   // Now check all the basic blocks in this live segment.
   MachineFunction::const_iterator MFI = MBB->getIterator();
   // Is this live segment the beginning of a non-PHIDef VN?
   if (S.start == VNI->def && !VNI->isPHIDef()) {
     // Not live-in to any blocks.
     if (MBB == EndMBB)
       return;

llvm/unittests/MI/LiveIntervalTest.cpp

 typedef std::function<void(MachineFunction&,LiveIntervals&)> LiveIntervalTest;
 
 struct TestPass : public MachineFunctionPass {
   static char ID;
   TestPass() : MachineFunctionPass(ID) {
     // We should never call this but always use PM.add(new TestPass(...))
     abort();
   }
-  TestPass(LiveIntervalTest T) : MachineFunctionPass(ID), T(T) {
+  TestPass(LiveIntervalTest T, bool ShouldPass)
+      : MachineFunctionPass(ID), T(T), ShouldPass(ShouldPass) {
     initializeTestPassPass(*PassRegistry::getPassRegistry());
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     LiveIntervals &LIS = getAnalysis<LiveIntervals>();
     T(MF, LIS);
-    EXPECT_TRUE(MF.verify(this));
+    EXPECT_EQ(MF.verify(this, /* Banner */ nullptr, /* AbortOnError */ false),
+              ShouldPass);
     return true;
   }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesAll();
     AU.addRequired<LiveIntervals>();
     AU.addPreserved<LiveIntervals>();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 private:
   LiveIntervalTest T;
+  bool ShouldPass;
 };
 
 static MachineInstr &getMI(MachineFunction &MF, unsigned At,
                            unsigned BlockNum) {
   MachineBasicBlock &MBB = *MF.getBlockNumbered(BlockNum);
 
   unsigned I = 0;
   for (MachineInstr &MI : MBB) {
static bool checkRegUnitInterference(LiveIntervals &LIS,
   for (MCRegUnitIterator Units(PhysReg, &TRI); Units.isValid(); ++Units) {
     const LiveRange &UnitRange = LIS.getRegUnit(*Units);
     if (VirtReg.overlaps(UnitRange, CP, *LIS.getSlotIndexes()))
       return true;
   }
   return false;
 }
 
-static void liveIntervalTest(StringRef MIRFunc, LiveIntervalTest T) {
+static void liveIntervalTest(StringRef MIRFunc, LiveIntervalTest T,
+                             bool ShouldPass = true) {
   LLVMContext Context;
   std::unique_ptr<LLVMTargetMachine> TM = createTargetMachine();
   // This test is designed for the X86 backend; stop if it is not available.
   if (!TM)
     return;
 
   legacy::PassManager PM;
 
   SmallString<160> S;
   StringRef MIRString = (Twine(R"MIR(
 ---
 ...
 name: func
 registers:
   - { id: 0, class: sreg_64 }
 body: |
   bb.0:
 )MIR") + Twine(MIRFunc) + Twine("...\n")).toNullTerminatedStringRef(S);
   std::unique_ptr<MIRParser> MIR;
   std::unique_ptr<Module> M = parseMIR(Context, PM, MIR, *TM, MIRString,
                                        "func");
   ASSERT_TRUE(M);
 
-  PM.add(new TestPass(T));
+  PM.add(new TestPass(T, ShouldPass));
 
   PM.run(*M);
 }
 
 } // End of anonymous namespace.
 
 char TestPass::ID = 0;
 INITIALIZE_PASS(TestPass, "testpass", "testpass", false, false)
)MIR",
 
         ASSERT_FALSE(checkRegUnitInterference(
             LIS, *MF.getSubtarget().getRegisterInfo(), R1, V1));
         ASSERT_FALSE(checkRegUnitInterference(
             LIS, *MF.getSubtarget().getRegisterInfo(), R2, V1));
       });
 }
 
+TEST(LiveIntervalTest, LiveThroughSegments) {
+  liveIntervalTest(
+      R"MIR(
+    %0 = IMPLICIT_DEF
+    S_BRANCH %bb.2
+  bb.1:
+    S_NOP 0, implicit %0
+    S_ENDPGM 0
+  bb.2:
+    S_BRANCH %bb.1
+)MIR",
+      [](MachineFunction &MF, LiveIntervals &LIS) {
+        MachineInstr &ImpDef = getMI(MF, 0, 0);
+        MachineInstr &Nop = getMI(MF, 0, 1);
+        LiveInterval &LI = LIS.getInterval(ImpDef.getOperand(0).getReg());
+        SlotIndex OrigIdx = LIS.getInstructionIndex(ImpDef).getRegSlot();
+        LiveInterval::iterator FirstSeg = LI.FindSegmentContaining(OrigIdx);
+
+        // %0 is live through bb.2. Move its def into bb.1 and update LIS but do
+        // not remove the segment for bb.2. This should cause machine
+        // verification to fail.
+        LIS.RemoveMachineInstrFromMaps(ImpDef);
+        ImpDef.moveBefore(&Nop);
+        LIS.InsertMachineInstrInMaps(ImpDef);
+
+        SlotIndex NewIdx = LIS.getInstructionIndex(ImpDef).getRegSlot();
+        FirstSeg->start = NewIdx;
+        FirstSeg->valno->def = NewIdx;
+      },
+      false);
+}
+
 int main(int argc, char **argv) {
   ::testing::InitGoogleTest(&argc, argv);
   initLLVM();
   return RUN_ALL_TESTS();
 }