CodeGen: Assert that liveness is up to date when reading block live-ins.

[MatzeB]

Summary

Add an assert that checks whether liveins are up to date before
they are used.

• Do not print liveins into .mir files anymore in situations where they are out of date anyway.
• The assert in the RegisterScavenger is superseded by the new one in livein_begin().
• Skip parts of the liveness updating logic in IfConversion.cpp when liveness isn't tracked anymore (just enough to avoid hitting the new assert()).

[MatzeB]

I just realized that I only tested AArch64 for this. Most of the other targets run into the newly introduced assert :-( So this needs further work before it can land.

[MatzeB]

Turns out not computing live-in lists when trackLivenessAfterRegAlloc() is too aggressive for todays LLVM (for example MachineLICM which is setup as part of TargetPassConfig::addOptimizedRegAlloc() requires up-to-date live-in lists.

In any case adding an assert when querying the live-in list should avoid bad usage.

I updated the patch accordingly.

[MatzeB]

bring back accidentally removed comment.

[MatzeB]

ping

[qcolombet]

Turns out not computing live-in lists when trackLivenessAfterRegAlloc() is too aggressive for todays LLVM (for example MachineLICM which is setup as part of TargetPassConfig::addOptimizedRegAlloc() requires up-to-date live-in lists.

Could you elaborate on the implications?
I stopped reviewing the patch when you put this comment but I didn't see any change on MachineLICM for instance, so I thought something bigger was still coming, but given the ping, it seems I missed the purpose of the comment x).

lib/CodeGen/RegisterScavenging.cpp

54

Removing that assert seems strange.
I was thinking this is okay because it will be checked anyway when we use MachineBasicBlock::liveins(), but was this your intent?

[MatzeB]

In D27562#627387, @qcolombet wrote:

Turns out not computing live-in lists when trackLivenessAfterRegAlloc() is too aggressive for todays LLVM (for example MachineLICM which is setup as part of TargetPassConfig::addOptimizedRegAlloc() requires up-to-date live-in lists.

Could you elaborate on the implications?
I stopped reviewing the patch when you put this comment but I didn't see any change on MachineLICM for instance, so I thought something bigger was still coming, but given the ping, it seems I missed the purpose of the comment x).

That comment was just about what changed in the updated version of the patch: My previous patch was more aggressive and disabled all live-in information if tracksLivenessAfterRegAlloc() was false. Turned out that doesn't work yet, so I left that for another day/patch.

This patch now just adds an assert and fixes instance where the block live-in lists are used in situations in which they are possibly out of date.

[MatzeB]

lib/CodeGen/RegisterScavenging.cpp

54

Yes, this assert() is redundant now because we have one in MachineBasicBlock::livein_begin() anyway.

[qcolombet]

Thanks for the clarifications Matthias.

LGTM.

Cheers,
-Quentin

include/llvm/CodeGen/MachineBasicBlock.h

public:
   void removeLiveIn(MCPhysReg Reg, LaneBitmask LaneMask = ~0u);
 
   /// Return true if the specified register is in the live in set.
   bool isLiveIn(MCPhysReg Reg, LaneBitmask LaneMask = ~0u) const;
 
   // Iteration support for live in sets.  These sets are kept in sorted
   // order by their register number.
   typedef LiveInVector::const_iterator livein_iterator;
-  livein_iterator livein_begin() const { return LiveIns.begin(); }
+  livein_iterator livein_begin() const;
   livein_iterator livein_end()   const { return LiveIns.end(); }
   bool            livein_empty() const { return LiveIns.empty(); }
   iterator_range<livein_iterator> liveins() const {
     return make_range(livein_begin(), livein_end());
   }
 
   /// Get the clobber mask for the start of this basic block. Funclets use this
   /// to prevent register allocation across funclet transitions.

lib/CodeGen/IfConversion.cpp

bool IfConverter::IfConvertSimple(BBInfo &BBI, IfcvtKind Kind) {
   if (CvtMBB.hasAddressTaken())
     // Conservatively abort if-conversion if BB's address is taken.
     return false;
 
   if (Kind == ICSimpleFalse)
     if (TII->reverseBranchCondition(Cond))
       llvm_unreachable("Unable to reverse branch condition!");
 
+  Redefs.init(*TRI);
+  DontKill.init(*TRI);
+
+  if (MRI->tracksLiveness()) {
     // Initialize liveins to the first BB. These are potentiall redefined by
     // predicated instructions.
-  Redefs.init(*TRI);
     Redefs.addLiveIns(CvtMBB);
     Redefs.addLiveIns(NextMBB);
-
     // Compute a set of registers which must not be killed by instructions in
     // BB1: This is everything live-in to BB2.
-  DontKill.init(*TRI);
     DontKill.addLiveIns(NextMBB);
+  }
 
   if (CvtMBB.pred_size() > 1) {
     BBI.NonPredSize -= TII->removeBranch(*BBI.BB);
     // Copy instructions in the true block, predicate them, and add them to
     // the entry block.
     CopyAndPredicateBlock(BBI, *CvtBBI, Cond);
 
     // RemoveExtraEdges won't work if the block has an unanalyzable branch, so
if (reverseBranchCondition(*CvtBBI)) {
         }
       }
     }
   }
 
   // Initialize liveins to the first BB. These are potentially redefined by
   // predicated instructions.
   Redefs.init(*TRI);
+  if (MRI->tracksLiveness()) {
     Redefs.addLiveIns(CvtMBB);
     Redefs.addLiveIns(NextMBB);
+  }
 
   DontKill.clear();
 
   bool HasEarlyExit = CvtBBI->FalseBB != nullptr;
   BranchProbability CvtNext, CvtFalse, BBNext, BBCvt;
 
   if (HasEarlyExit) {
     // Get probabilities before modifying CvtMBB and BBI.BB.
bool IfConverter::IfConvertDiamondCommon(
 
   // Initialize the Redefs:
   // - BB2 live-in regs need implicit uses before being redefined by BB1
   //   instructions.
   // - BB1 live-out regs need implicit uses before being redefined by BB2
   //   instructions. We start with BB1 live-ins so we have the live-out regs
   //   after tracking the BB1 instructions.
   Redefs.init(*TRI);
+  if (MRI->tracksLiveness()) {
     Redefs.addLiveIns(MBB1);
     Redefs.addLiveIns(MBB2);
+  }
 
   // Remove the duplicated instructions at the beginnings of both paths.
   // Skip dbg_value instructions
   MachineBasicBlock::iterator DI1 = MBB1.getFirstNonDebugInstr();
   MachineBasicBlock::iterator DI2 = MBB2.getFirstNonDebugInstr();
   BBI1->NonPredSize -= NumDups1;
   BBI2->NonPredSize -= NumDups1;
 
   // Skip past the dups on each side separately since there may be
   // differing dbg_value entries.
   for (unsigned i = 0; i < NumDups1; ++DI1) {
     if (!DI1->isDebugValue())
       ++i;
   }
   while (NumDups1 != 0) {
     ++DI2;
     if (!DI2->isDebugValue())
       --NumDups1;
   }
 
   // Compute a set of registers which must not be killed by instructions in BB1:
   // This is everything used+live in BB2 after the duplicated instructions. We
   // can compute this set by simulating liveness backwards from the end of BB2.
   DontKill.init(*TRI);
+  if (MRI->tracksLiveness()) {
     for (const MachineInstr &MI : make_range(MBB2.rbegin(), ++DI2.getReverse()))
       DontKill.stepBackward(MI);
 
     for (const MachineInstr &MI : make_range(MBB1.begin(), DI1)) {
-    SmallVector<std::pair<unsigned, const MachineOperand*>, 4> IgnoredClobbers;
-    Redefs.stepForward(MI, IgnoredClobbers);
+      SmallVector<std::pair<unsigned, const MachineOperand*>, 4> Dummy;
+      Redefs.stepForward(MI, Dummy);
+    }
   }
   BBI.BB->splice(BBI.BB->end(), &MBB1, MBB1.begin(), DI1);
   MBB2.erase(MBB2.begin(), DI2);
 
   // The branches have been checked to match, so it is safe to remove the branch
   // in BB1 and rely on the copy in BB2
 #ifndef NDEBUG
   // Unanalyzable branches must match exactly. Check that now.

lib/CodeGen/MIRPrinter.cpp

for (auto I = MBB.succ_begin(), E = MBB.succ_end(); I != E; ++I) {
            << format("0x%08" PRIx32, MBB.getSuccProbability(I).getNumerator())
            << ')';
     }
     OS << "\n";
     HasLineAttributes = true;
   }
 
   // Print the live in registers.
-  const auto *TRI = MBB.getParent()->getSubtarget().getRegisterInfo();
-  assert(TRI && "Expected target register info");
-  if (!MBB.livein_empty()) {
+  const MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
+  if (MRI.tracksLiveness() && !MBB.livein_empty()) {
+    const TargetRegisterInfo &TRI = *MRI.getTargetRegisterInfo();
     OS.indent(2) << "liveins: ";
     bool First = true;
     for (const auto &LI : MBB.liveins()) {
       if (!First)
         OS << ", ";
       First = false;
-      printReg(LI.PhysReg, OS, TRI);
+      printReg(LI.PhysReg, OS, &TRI);
       if (LI.LaneMask != ~0u)
         OS << ":0x" << PrintLaneMask(LI.LaneMask);
     }
     OS << "\n";
     HasLineAttributes = true;
   }
 
   if (HasLineAttributes)

lib/CodeGen/MachineBasicBlock.cpp

MachineBasicBlock::getEndClobberMask(const TargetRegisterInfo *TRI) const {
   // which does not preserve any registers. If there are no successors, we don't
   // care what kind of return it is, putting a mask after it is a no-op.
   return isReturnBlock() && !succ_empty() ? TRI->getNoPreservedMask() : nullptr;
 }
 
 void MachineBasicBlock::clearLiveIns() {
   LiveIns.clear();
 }
+
+MachineBasicBlock::livein_iterator MachineBasicBlock::livein_begin() const {
+  assert(getParent()->getProperties().hasProperty(
+      MachineFunctionProperties::Property::TracksLiveness) &&
+      "Liveness information is accurate");
+  return LiveIns.begin();
+}

lib/CodeGen/MachineVerifier.cpp

static bool matchPair(MachineBasicBlock::const_succ_iterator i,
   return false;
 }
 
 void
 MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
   FirstTerminator = nullptr;
 
   if (!MF->getProperties().hasProperty(
-      MachineFunctionProperties::Property::NoPHIs)) {
+      MachineFunctionProperties::Property::NoPHIs) && MRI->tracksLiveness()) {
     // If this block has allocatable physical registers live-in, check that
     // it is an entry block or landing pad.
     for (const auto &LI : MBB->liveins()) {
       if (isAllocatable(LI.PhysReg) && !MBB->isEHPad() &&
           MBB->getIterator() != MBB->getParent()->begin()) {
         report("MBB has allocable live-in, but isn't entry or landing-pad.", MBB);
       }
     }
if (!TBB && !FBB) {
                "condition!", MBB);
       }
     } else {
       report("AnalyzeBranch returned invalid data!", MBB);
     }
   }
 
   regsLive.clear();
+  if (MRI->tracksLiveness()) {
     for (const auto &LI : MBB->liveins()) {
       if (!TargetRegisterInfo::isPhysicalRegister(LI.PhysReg)) {
         report("MBB live-in list contains non-physical register", MBB);
         continue;
       }
       for (MCSubRegIterator SubRegs(LI.PhysReg, TRI, /*IncludeSelf=*/true);
            SubRegs.isValid(); ++SubRegs)
         regsLive.insert(*SubRegs);
     }
+  }
   regsLiveInButUnused = regsLive;
 
   const MachineFrameInfo &MFI = MF->getFrameInfo();
   BitVector PR = MFI.getPristineRegs(*MF);
   for (int I = PR.find_first(); I>0; I = PR.find_next(I)) {
     for (MCSubRegIterator SubRegs(I, TRI, /*IncludeSelf=*/true);
          SubRegs.isValid(); ++SubRegs)
       regsLive.insert(*SubRegs);

lib/CodeGen/RegisterScavenging.cpp

void RegScavenger::init(MachineBasicBlock &MBB) {
   MachineFunction &MF = *MBB.getParent();
   TII = MF.getSubtarget().getInstrInfo();
   TRI = MF.getSubtarget().getRegisterInfo();
   MRI = &MF.getRegInfo();
 
   assert((NumRegUnits == 0 || NumRegUnits == TRI->getNumRegUnits()) &&
          "Target changed?");
 
-  // It is not possible to use the register scavenger after late optimization
-  // passes that don't preserve accurate liveness information.
-  assert(MRI->tracksLiveness() &&
-         "Cannot use register scavenger with inaccurate liveness");
-
   // Self-initialize.
   if (!this->MBB) {
     NumRegUnits = TRI->getNumRegUnits();
     RegUnitsAvailable.resize(NumRegUnits);
     KillRegUnits.resize(NumRegUnits);
     DefRegUnits.resize(NumRegUnits);
     TmpRegUnits.resize(NumRegUnits);
   }

test/CodeGen/AArch64/machine-scheduler.mir

--- |
   }
 ...
 ---
 # CHECK-LABEL: name: load_imp-def
 # CHECK: bb.0.entry:
 # CHECK: LDRWui %x0, 0
 # CHECK: LDRWui %x0, 1
 # CHECK: STRWui %w1, %x0, 2
-name:            load_imp-def
-body:             |
+name: load_imp-def
+tracksRegLiveness: true
+body: |
   bb.0.entry:
     liveins: %w1, %x0
     %w8 = LDRWui %x0, 1, implicit-def %x8  :: (load 4 from %ir.0)
     STRWui killed %w1, %x0, 2 :: (store 4 into %ir.arrayidx1)
     %w9 = LDRWui killed %x0, 0, implicit-def %x9  :: (load 4 from %ir.arrayidx19, align 8)
     %x0 = ADDXrr killed %x9, killed %x8
     RET_ReallyLR implicit %x0
 ...
 

test/CodeGen/MIR/X86/basic-block-liveins.mir

--- |
 
   define i32 @test3() {
   body:
     ret i32 0
   }
 
 ...
 ---
-name:            test
+name: test
+tracksRegLiveness: true
 body: |
   ; CHECK-LABEL: bb.0.body:
   ; CHECK-NEXT:    liveins: %edi, %esi
   bb.0.body:
     liveins: %edi, %esi
 
     %eax = LEA64_32r killed %rdi, 1, killed %rsi, 0, _
     RETQ %eax
 ...
 ---
-name:            test2
+name: test2
+tracksRegLiveness: true
 body: |
   ; CHECK-LABEL: name: test2
   ; Verify that we can have multiple lists of liveins that will be merged into
   ; one.
   ; CHECK:       bb.0.body:
   ; CHECK-NEXT:    liveins: %edi, %esi
   bb.0.body:
     liveins: %edi
     liveins: %esi
 
     %eax = LEA64_32r killed %rdi, 1, killed %rsi, 0, _
     RETQ %eax
 ...
 ---
-name:            test3
+name: test3
+tracksRegLiveness: true
 body: |
   ; Verify that we can have an empty list of liveins.
   ; CHECK-LABEL: name: test3
   ; CHECK:       bb.0.body:
   ; CHECK-NEXT:    %eax = MOV32r0 implicit-def dead %eflags
   bb.0.body:
     liveins:
 
     %eax = MOV32r0 implicit-def dead %eflags
     RETQ killed %eax
 ...

test/CodeGen/MIR/X86/machine-verifier.mir

 # RUN: not llc -march=x86-64 -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
 # This test ensures that the MIR parser runs the machine verifier after parsing.
 
 --- |
 
   define i32 @inc(i32 %a) {
   entry:
     ret i32 %a
   }
 
 ...
 ---
-name:            inc
+name: inc
+tracksRegLiveness: true
 body: |
   bb.0.entry:
     liveins: %edi
    ; CHECK: *** Bad machine code: Too few operands ***
    ; CHECK: instruction: COPY
    ; CHECK: 2 operands expected, but 0 given.
     COPY
     RETQ
 ...

test/CodeGen/X86/tail-call-conditional.mir

define i64 @test(i64 %arg, i8* %arg1) optsize {
     ret i64 123
   }
 
   declare i64 @f1(i8*, i64)
   declare i64 @f2(i8*, i64)
 
 ...
 ---
-name:            test
+name: test
+tracksRegLiveness: true
 liveins:
   - { reg: '%rdi' }
   - { reg: '%rsi' }
 body:             |
   bb.0:
     successors: %bb.1, %bb.4
     liveins: %rdi, %rsi