[MachineLICM] Assert that locations from debug insts are not lost

[vsk]

Summary

Assert that MachineLICM does not move a debug instruction and then drop
its debug location. Later passes require each debug instruction to have
a location.

Testing: check-llvm, clang stage2 RelWithDebInfo build (x86_64)

[davide]

As we discussed offline, I think the assertion makes sense, so LGTM. It's good to catch violations while we decide what to do with hoisting (what are the semantics et similia).
Do you think it's worth writing a verifier check to make sure that DBG_VALUE(s) when hoisted have always a location -- otherwise the MIR is considered illformed? I can do that, but I would like to have your opinion on it.

[vsk]

In D80665#2058533, @davide wrote:

Do you think it's worth writing a verifier check to make sure that DBG_VALUE(s) when hoisted have always a location -- otherwise the MIR is considered illformed? I can do that, but I would like to have your opinion on it.

That's a good point. I went ahead and took a stab at this: https://reviews.llvm.org/D80670.

[jmorse]

LGTM -- I'd suggest that the assertion messages could become something like "Shouldn't be hoisting debug instructions" or similar. IMO, as well as the matter of dropping DebugLocs, debug instructions should (almost?) never appears as a candidate to be moved around like another instruction.

[davide]

In D80665#2060766, @jmorse wrote:

LGTM -- I'd suggest that the assertion messages could become something like "Shouldn't be hoisting debug instructions" or similar. IMO, as well as the matter of dropping DebugLocs, debug instructions should (almost?) never appears as a candidate to be moved around like another instruction.

While I agree with you in theory, the classical counterexample for this claim is merging successors in predecessors, or all the CFG'ish transformations that SimplifyCFG is very good at doing. In that case you just move full blocks around and the likelihood of moving DebugInfo around is very high.

[davide]

In D80665#2060894, @davide wrote:

In D80665#2060766, @jmorse wrote:

LGTM -- I'd suggest that the assertion messages could become something like "Shouldn't be hoisting debug instructions" or similar. IMO, as well as the matter of dropping DebugLocs, debug instructions should (almost?) never appears as a candidate to be moved around like another instruction.

While I agree with you in theory, the classical counterexample for this claim is merging successors in predecessors, or all the CFG'ish transformations that SimplifyCFG is very good at doing. In that case you just move full blocks around and the likelihood of moving DebugInfo around is very high.

LICM and MachineLICM really don't quite suffer from this problem, as you already pointed out.

[vsk]

In D80665#2060766, @jmorse wrote:

LGTM -- I'd suggest that the assertion messages could become something like "Shouldn't be hoisting debug instructions" or similar. IMO, as well as the matter of dropping DebugLocs, debug instructions should (almost?) never appears as a candidate to be moved around like another instruction.

Thanks, I'll fix this before committing.

llvm/lib/CodeGen/MachineLICM.cpp

void MachineLICMBase::HoistPostRA(MachineInstr *MI, unsigned Def) {
 
   // Splice the instruction to the preheader.
   MachineBasicBlock *MBB = MI->getParent();
   Preheader->splice(Preheader->getFirstTerminator(), MBB, MI);
 
   // Since we are moving the instruction out of its basic block, we do not
   // retain its debug location. Doing so would degrade the debugging
   // experience and adversely affect the accuracy of profiling information.
+  assert(!MI->isDebugInstr() && "Should not hoist debug inst");
   MI->setDebugLoc(DebugLoc());
 
   // Add register to livein list to all the BBs in the current loop since a
   // loop invariant must be kept live throughout the whole loop. This is
   // important to ensure later passes do not scavenge the def register.
   AddToLiveIns(Def);
 
   ++NumPostRAHoisted;
if (!CanSink || !B || B == Preheader)
       continue;
 
     LLVM_DEBUG(dbgs() << "Sinking to " << printMBBReference(*B) << " from "
                       << printMBBReference(*I->getParent()) << ": " << *I);
     B->splice(B->getFirstNonPHI(), Preheader, I);
 
     // The instruction is is moved from its basic block, so do not retain the
     // debug information.
+    assert(!I->isDebugInstr() && "Should not sink debug inst");
     I->setDebugLoc(DebugLoc());
   }
 }
 
 static bool isOperandKill(const MachineOperand &MO, MachineRegisterInfo *MRI) {
   return MO.isKill() || MRI->hasOneNonDBGUse(MO.getReg());
 }
 
DenseMap<unsigned, std::vector<const MachineInstr *>>::iterator
     CI = CSEMap.find(Opcode);
   if (!EliminateCSE(MI, CI)) {
     // Otherwise, splice the instruction to the preheader.
     Preheader->splice(Preheader->getFirstTerminator(),MI->getParent(),MI);
 
     // Since we are moving the instruction out of its basic block, we do not
     // retain its debug location. Doing so would degrade the debugging
     // experience and adversely affect the accuracy of profiling information.
+    assert(!MI->isDebugInstr() && "Should not hoist debug inst");
     MI->setDebugLoc(DebugLoc());
 
     // Update register pressure for BBs from header to this block.
     UpdateBackTraceRegPressure(MI);
 
     // Clear the kill flags of any register this instruction defines,
     // since they may need to be live throughout the entire loop
     // rather than just live for part of it.