[DebugInfo][RemoveDIs] Eliminate some debug-intrinsics-affect-codegen errors

[jmorse]

Summary

This is a patch to reduce the effect that debug intrinsics have on codegen -- there are a few flavours of defect delt with here:

• Setting the DebugLoc of an instruction to one derived from a debug instruction,
• Counting the size of a block to include debug intrinsics,
• Examining debug intrinsics in SLPVectoriser (they should all be ignored)
• Picking an insertion point at a debug intrinsic in SCEVExpander

Not included in this patch are a few scenarios where we insert an existing instruction at a debug intrinsic position: these can have meaningful effects on the stepping behaviour, so we don't want to address them now. The two SCEVExpander scenarios are where the pass is generating new code and doesn't have an opinion on the line numbers.

I haven't written tests for these changes: IMO these changes are obviously correct, and I'd prefer to not test for behaviours that we're actively trying to design away.

[dblaikie]

I know I'm a bit of a stickler for it, but my vote would be/I'd still prefer to see test cases for these. To demonstrate the code is exercised/correct, if nothing else. I realize the chance of regressing these fixes is probably relatively small, but I've certainly come across code before and been "I'm not sure why this is here, let's remove it and see what tests fail" in an effort to understand the code.

[jryans]

llvm/lib/IR/BasicBlock.cpp

423–424

It might be to structure this as an if ... else ... so we always test isa<DbgInfoIntrinsic>(&*I) first, instead of unconditionally calling I->getDebugLoc() and then potentially replacing it...? (If you agree, then the same comment applies to quite a few other changes here.)

[fdeazeve]

llvm/lib/IR/BasicBlock.cpp

423–424

Not to turn this into a bike-shedding discussion but this seems like an important enough pattern that some helper function could live alongside Intrusction::getNextNonDebugInstruction and friends? Something similar to BasicBlock::skipDebugIntrinsics.

I only mention this because an if/else pattern would necessarily imply declaring an uninitialized loc or having a big if/else block, both of which are undesirable.

[jmorse]

(NB, this is still high on my priority list, but I'm swimming through a lot of code-soup right now, will be back on this in a day or two)

[jmorse]

Will shoe-horn in a utility for easier setDebugLoc usage in the next revision, to be clear though, the intention is to delete all this code in the future if we're living in an intrinsic-free paradise by then,

In D151419#4373747, @dblaikie wrote:

I know I'm a bit of a stickler for it, but my vote would be/I'd still prefer to see test cases for these. To demonstrate the code is exercised/correct, if nothing else. I realize the chance of regressing these fixes is probably relatively small, but I've certainly come across code before and been "I'm not sure why this is here, let's remove it and see what tests fail" in an effort to understand the code.

This feeds into the other ~30 patches that will appear in this series: as it stands, for a large number of optimisation passes we don't test for the exact placement of debug intrinsics (but we should). Seeing how we're planning on changing the mechanism via which debug-info is preserved, through putting more information in iterators [0], if we went for the every-functional-change-gets-a-test approach then this would expand to fully testing debug-info in every optimisation pass.

I really want to improve the testing in this area (we've got ~130 LLVM-IR inputs that are sensitive to our changes that can be promoted into being tests) and the whole area is under-tested as it stands -- but I'd prefer to put the time into testing the later patches, instead of these specific changes where the ultimate goal is to design-out the behaviour. Ideally in ~six months any tests that would be added in this patch could be deleted!

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939

[dblaikie]

In D151419#4405466, @jmorse wrote:

Will shoe-horn in a utility for easier setDebugLoc usage in the next revision, to be clear though, the intention is to delete all this code in the future if we're living in an intrinsic-free paradise by then,

In D151419#4373747, @dblaikie wrote:

I know I'm a bit of a stickler for it, but my vote would be/I'd still prefer to see test cases for these. To demonstrate the code is exercised/correct, if nothing else. I realize the chance of regressing these fixes is probably relatively small, but I've certainly come across code before and been "I'm not sure why this is here, let's remove it and see what tests fail" in an effort to understand the code.

This feeds into the other ~30 patches that will appear in this series: as it stands, for a large number of optimisation passes we don't test for the exact placement of debug intrinsics (but we should). Seeing how we're planning on changing the mechanism via which debug-info is preserved, through putting more information in iterators [0], if we went for the every-functional-change-gets-a-test approach then this would expand to fully testing debug-info in every optimisation pass.

I really want to improve the testing in this area (we've got ~130 LLVM-IR inputs that are sensitive to our changes that can be promoted into being tests) and the whole area is under-tested as it stands -- but I'd prefer to put the time into testing the later patches, instead of these specific changes where the ultimate goal is to design-out the behaviour. Ideally in ~six months any tests that would be added in this patch could be deleted!

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939

Fair enough, though I'm not especially comfortable with it.

[jmorse]

Add a "getStableDebugLoc" getter in addition to getDebugLoc, the difference being that the former skips debug intrinsics.

[jmorse]

I'm going to upload a few more patches and put them in a stack to illustrate the direction of change we're aiming for -- there are a bunch of open questions about how to go about testing them, similar to the discussion above, which'll probably be easier to describe once the changes are visible.

[probinson]

IIUC you're wanting to not add new tests that check for correct intrinsic-call placement, on the grounds that intrinsics are going away and those tests would ultimately be deleted.
If the tests are deleted, then how do we verify that the debug info (in its new form) is correct? The transforms will still transform the IR, and it would be very helpful when trying to track down future debug-info bugs if we still have a way to follow what the brave new world has in it. -dump-after-all is pretty handy in some of these cases, but if it stops showing debug info then future bug analysis may become much more difficult.

Hopefully the promised related patches will demonstrate this,

[jmorse]

In D151419#4406678, @probinson wrote:

IIUC you're wanting to not add new tests that check for correct intrinsic-call placement, on the grounds that intrinsics are going away and those tests would ultimately be deleted.

Slight difference -- all the changes in this specific patch are for DebugLocs, to avoid "real" instructions setting their locations from debug-instructions. That legitimately will be designed away,

If the tests are deleted, then how do we verify that the debug info (in its new form) is correct? The transforms will still transform the IR, and it would be very helpful when trying to track down future debug-info bugs if we still have a way to follow what the brave new world has in it. -dump-after-all is pretty handy in some of these cases, but if it stops showing debug info then future bug analysis may become much more difficult.

I agree with the overall point that we need to preserve and improve testing of the placement of variable location debug-info, no matter the format. As it stands, in all the patches produced so far, there's a 1 <=> 1 correspondence between dbg.value intrinsics and the new forms we've been experimenting on, so that the two formats can be trivially switched between, and the existing tests can all work. We've also instrumented the printing-passes and asm writers to always write out dbg.values. IMO: that should continue to be the case through all this development, until we get to a discussion on how to textually represent this information, as that lets us talk purely about how to implement (and test) optimisations rather than everything else.

[jmorse]

NB: I've solicited some more feedback on the general testing approach here [0], although as with my previous post this specific patch is different, as it's to do with protecting DebugLocs from debug intrinsics rather than the placement of debug intrinsics.

[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939/6

[probinson]

Slight difference -- all the changes in this specific patch are for DebugLocs, to avoid "real" instructions setting their locations from debug-instructions. That legitimately will be designed away,

That's not what the patch description says; it's about eliminating codegen errors. Are you mixing two patches into one? I've commented on the ones that look like codegen fixes.

Now, about testing... like @dblaikie I'm not very comfortable with this no-testing business, although I see he accepted the patch already. Are the DebugLoc changes in fact NFC? Taking the DebugLoc from non-debug instructions changes nothing? I know we have next to no tests for this sort of thing, so maybe there's no immediate value in generating a bunch, but I wanted to know whether the intent is just to prep for eliminating debug instructions, or actually making things better in the source-location aspect.

As for the gnochange bits, those are functional changes and we will not reach the Promised Land of no debug instructions for at least one additional release. Given past contentiousness over IR transform tests and debug info, I'd feel a whole lot better if we had tests--with checks generated by the scripts that the IR-transform mavens want to use--to make sure we don't regress in the meantime.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

1889

Looks like a gnochange fix.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

5351

Double semicolon at the end.
Is this strictly a performance improvement? (in which case possibly the "Ignore debug intrinsics" loop a few lines above can go away?)
Or if we find a debug instruction does this prematurely break out of the loop (making it a gnochange bug)? I guess it depends on how isInsertedInstruction responds; I don't mess around inside optimizations enough to know.

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp

102

This looks like a gnochange fix.

150

This looks like a gnochange fix.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

3391

This looks like a gnochange fix.

3424

This looks like a gnochange fix.

11139

Another gnochange fix?

11287

This looks like a gnochange fix.

11329

This looks like a gnochange fix.

11342

This looks like a gnochange fix.

11443

This looks like a gnochange fix.

11463

This looks like a gnochange fix.

11486

This looks like a gnochange fix.

11570

This looks like a gnochange fix.

11610

This looks like a gnochange fix.

11638

This looks like a gnochange fix.

11653

This looks like a gnochange fix.

llvm/include/llvm/IR/Instruction.h

public:
   bool extractProfTotalWeight(uint64_t &TotalVal) const;
 
   /// Set the debug location information for this instruction.
   void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
 
   /// Return the debug location for this node as a DebugLoc.
   const DebugLoc &getDebugLoc() const { return DbgLoc; }
 
+  /// Fetch the debug location for this node, unless this is a debug intrinsic,
+  /// in which case fetch the debug location of the next non-debug node.
+  const DebugLoc &getStableDebugLoc() const;
+
   /// Set or clear the nuw flag on this instruction, which must be an operator
   /// which supports this flag. See LangRef.html for the meaning of this flag.
   void setHasNoUnsignedWrap(bool b = true);
 
   /// Set or clear the nsw flag on this instruction, which must be an operator
   /// which supports this flag. See LangRef.html for the meaning of this flag.
   void setHasNoSignedWrap(bool b = true);
 

llvm/lib/IR/BasicBlock.cpp

BasicBlock *BasicBlock::splitBasicBlock(iterator I, const Twine &BBName,
   assert(getTerminator() && "Can't use splitBasicBlock on degenerate BB!");
   assert(I != InstList.end() &&
          "Trying to get me to create degenerate basic block!");
 
   BasicBlock *New = BasicBlock::Create(getContext(), BBName, getParent(),
                                        this->getNextNode());
 
   // Save DebugLoc of split point before invalidating iterator.
-  DebugLoc Loc = I->getDebugLoc();
+  DebugLoc Loc = I->getStableDebugLoc();
   // Move all of the specified instructions from the original basic block into
   // the new basic block.
   New->splice(New->end(), this, I, end());
 
   // Add a branch instruction to the newly formed basic block.
   BranchInst *BI = BranchInst::Create(New, this);
   BI->setDebugLoc(Loc);
 
   // Now we must loop through all of the successors of the New block (which

llvm/lib/IR/Instruction.cpp

 const Instruction *
 Instruction::getPrevNonDebugInstruction(bool SkipPseudoOp) const {
   for (const Instruction *I = getPrevNode(); I; I = I->getPrevNode())
     if (!isa<DbgInfoIntrinsic>(I) && !(SkipPseudoOp && isa<PseudoProbeInst>(I)))
       return I;
   return nullptr;
 }
 
+const DebugLoc &Instruction::getStableDebugLoc() const {
+  if (isa<DbgInfoIntrinsic>(this))
+    return getNextNonDebugInstruction()->getDebugLoc();
+  return getDebugLoc();
+}
+
 bool Instruction::isAssociative() const {
   unsigned Opcode = getOpcode();
   if (isAssociative(Opcode))
     return true;
 
   switch (Opcode) {
   case FMul:
   case FAdd:

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp

bool LoopIdiomRecognize::recognizePopcount() {
   // non-compact loop. Therefore, recognizing popcount idiom only makes sense
   // in a compact loop.
 
   // Give up if the loop has multiple blocks or multiple backedges.
   if (CurLoop->getNumBackEdges() != 1 || CurLoop->getNumBlocks() != 1)
     return false;
 
   BasicBlock *LoopBody = *(CurLoop->block_begin());
-  if (LoopBody->size() >= 20) {
+  if (LoopBody->sizeWithoutDebug() >= 20) {
     // The loop is too big, bail out.
     return false;
   }
 
   // It should have a preheader containing nothing but an unconditional branch.
   BasicBlock *PH = CurLoop->getLoopPreheader();
   if (!PH || &PH->front() != PH->getTerminator())
     return false;

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

BasicBlock::iterator LSRInstance::AdjustInsertPositionForExpand(
 
   // Ignore debug intrinsics.
   while (isa<DbgInfoIntrinsic>(IP)) ++IP;
 
   // Set IP below instructions recently inserted by SCEVExpander. This keeps the
   // IP consistent across expansions and allows the previously inserted
   // instructions to be reused by subsequent expansion.
   while (Rewriter.isInsertedInstruction(&*IP) && IP != LowestIP)
-    ++IP;
+    IP = IP->getNextNonDebugInstruction()->getIterator();;
 
   return IP;
 }
 
 /// Emit instructions for the leading candidate expression for this LSRUse (this
 /// is called "expanding").
 Value *LSRInstance::Expand(const LSRUse &LU, const LSRFixup &LF,
                            const Formula &F, BasicBlock::iterator IP,

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

static void SplitLandingPadPredecessorsImpl(
   // it right before the original block.
   BasicBlock *NewBB1 = BasicBlock::Create(OrigBB->getContext(),
                                           OrigBB->getName() + Suffix1,
                                           OrigBB->getParent(), OrigBB);
   NewBBs.push_back(NewBB1);
 
   // The new block unconditionally branches to the old block.
   BranchInst *BI1 = BranchInst::Create(OrigBB, NewBB1);
-  BI1->setDebugLoc(OrigBB->getFirstNonPHI()->getDebugLoc());
+  BI1->setDebugLoc(OrigBB->getFirstNonPHIOrDbg()->getDebugLoc());
 
   // Move the edges from Preds to point to NewBB1 instead of OrigBB.
   for (BasicBlock *Pred : Preds) {
     // This is slightly more strict than necessary; the minimum requirement
     // is that there be no more than one indirectbr branching to BB. And
     // all BlockAddress uses would need to be updated.
     assert(!isa<IndirectBrInst>(Pred->getTerminator()) &&
            "Cannot split an edge from an IndirectBrInst");
if (!NewBB2Preds.empty()) {
     // Create another basic block for the rest of OrigBB's predecessors.
     NewBB2 = BasicBlock::Create(OrigBB->getContext(),
                                 OrigBB->getName() + Suffix2,
                                 OrigBB->getParent(), OrigBB);
     NewBBs.push_back(NewBB2);
 
     // The new block unconditionally branches to the old block.
     BranchInst *BI2 = BranchInst::Create(OrigBB, NewBB2);
-    BI2->setDebugLoc(OrigBB->getFirstNonPHI()->getDebugLoc());
+    BI2->setDebugLoc(OrigBB->getFirstNonPHIOrDbg()->getDebugLoc());
 
     // Move the remaining edges from OrigBB to point to NewBB2.
     for (BasicBlock *NewBB2Pred : NewBB2Preds)
       NewBB2Pred->getTerminator()->replaceUsesOfWith(OrigBB, NewBB2);
 
     // Update DominatorTree, LoopInfo, and LCCSA analysis information.
     HasLoopExit = false;
     UpdateAnalysisInformation(OrigBB, NewBB2, NewBB2Preds, DTU, DT, LI, MSSAU,

 static Instruction *
 SplitBlockAndInsertIfThenImpl(Value *Cond, Instruction *SplitBefore,
                               bool Unreachable, MDNode *BranchWeights,
                               DomTreeUpdater *DTU, DominatorTree *DT,
                               LoopInfo *LI, BasicBlock *ThenBlock) {
   SmallVector<DominatorTree::UpdateType, 8> Updates;
   BasicBlock *Head = SplitBefore->getParent();
   BasicBlock *Tail = Head->splitBasicBlock(SplitBefore->getIterator());
+  DebugLoc DbgLoc = SplitBefore->getStableDebugLoc();
   if (DTU) {
     SmallPtrSet<BasicBlock *, 8> UniqueSuccessorsOfHead;
     Updates.push_back({DominatorTree::Insert, Head, Tail});
     Updates.reserve(Updates.size() + 2 * succ_size(Tail));
     for (BasicBlock *SuccessorOfHead : successors(Tail))
       if (UniqueSuccessorsOfHead.insert(SuccessorOfHead).second) {
         Updates.push_back({DominatorTree::Insert, Tail, SuccessorOfHead});
         Updates.push_back({DominatorTree::Delete, Head, SuccessorOfHead});
       }
   }
   Instruction *HeadOldTerm = Head->getTerminator();
   LLVMContext &C = Head->getContext();
   Instruction *CheckTerm;
   bool CreateThenBlock = (ThenBlock == nullptr);
   if (CreateThenBlock) {
     ThenBlock = BasicBlock::Create(C, "", Head->getParent(), Tail);
     if (Unreachable)
       CheckTerm = new UnreachableInst(C, ThenBlock);
     else {
       CheckTerm = BranchInst::Create(Tail, ThenBlock);
       if (DTU)
         Updates.push_back({DominatorTree::Insert, ThenBlock, Tail});
     }
-    CheckTerm->setDebugLoc(SplitBefore->getDebugLoc());
+    CheckTerm->setDebugLoc(DbgLoc);
   } else
     CheckTerm = ThenBlock->getTerminator();
   BranchInst *HeadNewTerm =
       BranchInst::Create(/*ifTrue*/ ThenBlock, /*ifFalse*/ Tail, Cond);
   if (DTU)
     Updates.push_back({DominatorTree::Insert, Head, ThenBlock});
   HeadNewTerm->setMetadata(LLVMContext::MD_prof, BranchWeights);
   ReplaceInstWithInst(HeadOldTerm, HeadNewTerm);

 }
 
 void llvm::SplitBlockAndInsertIfThenElse(Value *Cond, Instruction *SplitBefore,
                                          Instruction **ThenTerm,
                                          Instruction **ElseTerm,
                                          MDNode *BranchWeights,
                                          DomTreeUpdater *DTU) {
   BasicBlock *Head = SplitBefore->getParent();
+  DebugLoc DbgLoc = SplitBefore->getStableDebugLoc();
 
   SmallPtrSet<BasicBlock *, 8> UniqueOrigSuccessors;
   if (DTU)
     UniqueOrigSuccessors.insert(succ_begin(Head), succ_end(Head));
 
   BasicBlock *Tail = Head->splitBasicBlock(SplitBefore->getIterator());
   Instruction *HeadOldTerm = Head->getTerminator();
   LLVMContext &C = Head->getContext();
   BasicBlock *ThenBlock = BasicBlock::Create(C, "", Head->getParent(), Tail);
   BasicBlock *ElseBlock = BasicBlock::Create(C, "", Head->getParent(), Tail);
   *ThenTerm = BranchInst::Create(Tail, ThenBlock);
-  (*ThenTerm)->setDebugLoc(SplitBefore->getDebugLoc());
+  (*ThenTerm)->setDebugLoc(DbgLoc);
   *ElseTerm = BranchInst::Create(Tail, ElseBlock);
-  (*ElseTerm)->setDebugLoc(SplitBefore->getDebugLoc());
+  (*ElseTerm)->setDebugLoc(DbgLoc);
   BranchInst *HeadNewTerm =
     BranchInst::Create(/*ifTrue*/ThenBlock, /*ifFalse*/ElseBlock, Cond);
   HeadNewTerm->setMetadata(LLVMContext::MD_prof, BranchWeights);
   ReplaceInstWithInst(HeadOldTerm, HeadNewTerm);
   if (DTU) {
     SmallVector<DominatorTree::UpdateType, 8> Updates;
     Updates.reserve(4 + 2 * UniqueOrigSuccessors.size());
     for (BasicBlock *Succ : successors(Head)) {

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp

assert(!isa<Instruction>(Ret) ||
          SE.DT.dominates(cast<Instruction>(Ret), &*BIP));
 
   return Ret;
 }
 
 BasicBlock::iterator
 SCEVExpander::findInsertPointAfter(Instruction *I,
                                    Instruction *MustDominate) const {
-  BasicBlock::iterator IP = ++I->getIterator();
+  BasicBlock::iterator IP = std::next(I->getIterator());
+  IP = skipDebugIntrinsics(IP);
   if (auto *II = dyn_cast<InvokeInst>(I))
     IP = II->getNormalDest()->begin();
 
+  IP = skipDebugIntrinsics(IP);
   while (isa<PHINode>(IP))
     ++IP;
 
   if (isa<FuncletPadInst>(IP) || isa<LandingPadInst>(IP)) {
     ++IP;
   } else if (isa<CatchSwitchInst>(IP)) {
     IP = MustDominate->getParent()->getFirstInsertionPt();
   } else {
     assert(!IP->isEHPad() && "unexpected eh pad!");
   }
 
   // Adjust insert point to be after instructions inserted by the expander, so
   // we can re-use already inserted instructions. Avoid skipping past the
   // original \p MustDominate, in case it is an inserted instruction.
   while (isInsertedInstruction(&*IP) && &*IP != MustDominate)
     ++IP;
 
+  IP = skipDebugIntrinsics(IP);
   return IP;
 }
 
 BasicBlock::iterator
 SCEVExpander::GetOptimalInsertionPointForCastOf(Value *V) const {
   // Cast the argument at the beginning of the entry block, after
   // any bitcasts of other arguments.
   if (Argument *A = dyn_cast<Argument>(V)) {
SCEVExpander::GetOptimalInsertionPointForCastOf(Value *V) const {
   // Cast the instruction immediately after the instruction.
   if (Instruction *I = dyn_cast<Instruction>(V))
     return findInsertPointAfter(I, &*Builder.GetInsertPoint());
 
   // Otherwise, this must be some kind of a constant,
   // so let's plop this cast into the function's entry block.
   assert(isa<Constant>(V) &&
          "Expected the cast argument to be a global/constant");
-  return Builder.GetInsertBlock()
+  return skipDebugIntrinsics(Builder.GetInsertBlock()
       ->getParent()
       ->getEntryBlock()
-      .getFirstInsertionPt();
+      .getFirstInsertionPt());
 }
 
 /// InsertNoopCastOfTo - Insert a cast of V to the specified type,
 /// which must be possible with a noop cast, doing what we can to share
 /// the casts.
 Value *SCEVExpander::InsertNoopCastOfTo(Value *V, Type *Ty) {
   Instruction::CastOps Op = CastInst::getCastOpcode(V, false, Ty, false);
   assert((Op == Instruction::BitCast ||

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

static bool mergeConditionalStoreToAddress(
   Value *QCond = cast<BranchInst>(QFB->getSinglePredecessor()->getTerminator())
                      ->getCondition();
 
   Value *PPHI = ensureValueAvailableInSuccessor(PStore->getValueOperand(),
                                                 PStore->getParent());
   Value *QPHI = ensureValueAvailableInSuccessor(QStore->getValueOperand(),
                                                 QStore->getParent(), PPHI);
 
-  IRBuilder<> QB(&*PostBB->getFirstInsertionPt());
+  Instruction *PostBBFirst = &*PostBB->getFirstInsertionPt();
+  IRBuilder<> QB(PostBBFirst);
+  QB.SetCurrentDebugLocation(PostBBFirst->getStableDebugLoc());
 
   Value *PPred = PStore->getParent() == PTB ? PCond : QB.CreateNot(PCond);
   Value *QPred = QStore->getParent() == QTB ? QCond : QB.CreateNot(QCond);
 
   if (InvertPCond)
     PPred = QB.CreateNot(PPred);
   if (InvertQCond)
     QPred = QB.CreateNot(QPred);

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp

struct BlockScheduling {
     void verify() {
       if (!ScheduleStart)
         return;
 
       assert(ScheduleStart->getParent() == ScheduleEnd->getParent() &&
              ScheduleStart->comesBefore(ScheduleEnd) &&
              "Not a valid scheduling region?");
 
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
         auto *SD = getScheduleData(I);
         if (!SD)
           continue;
         assert(isInSchedulingRegion(SD) &&
                "primary schedule data not in window?");
         assert(isInSchedulingRegion(SD->FirstInBundle) &&
                "entire bundle in window!");
         (void)SD;
void doForAllOpcodes(Value *V,
         for (auto &P : I->second)
           if (isInSchedulingRegion(P.second))
             Action(P.second);
     }
 
     /// Put all instructions into the ReadyList which are ready for scheduling.
     template <typename ReadyListType>
     void initialFillReadyList(ReadyListType &ReadyList) {
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
         doForAllOpcodes(I, [&](ScheduleData *SD) {
           if (SD->isSchedulingEntity() && SD->hasValidDependencies() &&
               SD->isReady()) {
             ReadyList.insert(SD);
             LLVM_DEBUG(dbgs()
                        << "SLP:    initially in ready list: " << *SD << "\n");
           }
         });
BoUpSLP::BlockScheduling::tryScheduleBundle(ArrayRef<Value *> VL, BoUpSLP *SLP,
   auto TryScheduleBundleImpl = [this, OldScheduleEnd, SLP](bool ReSchedule,
                                                          ScheduleData *Bundle) {
     // The scheduling region got new instructions at the lower end (or it is a
     // new region for the first bundle). This makes it necessary to
     // recalculate all dependencies.
     // It is seldom that this needs to be done a second time after adding the
     // initial bundle to the region.
     if (ScheduleEnd != OldScheduleEnd) {
-      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode())
+      for (auto *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction())
         doForAllOpcodes(I, [](ScheduleData *SD) { SD->clearDependencies(); });
       ReSchedule = true;
     }
     if (Bundle) {
       LLVM_DEBUG(dbgs() << "SLP: try schedule bundle " << *Bundle
                         << " in block " << BB->getName() << "\n");
       calculateDependencies(Bundle, /*InsertInReadyList=*/true, SLP);
     }
auto &&CheckScheduleForI = [this, &S](Instruction *I) -> bool {
     SD->init(SchedulingRegionID, S.OpValue);
     ExtraScheduleDataMap[I][S.OpValue] = SD;
     return true;
   };
   if (CheckScheduleForI(I))
     return true;
   if (!ScheduleStart) {
     // It's the first instruction in the new region.
-    initScheduleData(I, I->getNextNode(), nullptr, nullptr);
+    initScheduleData(I, I->getNextNonDebugInstruction(), nullptr, nullptr);
     ScheduleStart = I;
-    ScheduleEnd = I->getNextNode();
+    ScheduleEnd = I->getNextNonDebugInstruction();
     if (isOneOf(S, I) != I)
       CheckScheduleForI(I);
     assert(ScheduleEnd && "tried to vectorize a terminator?");
     LLVM_DEBUG(dbgs() << "SLP:  initialize schedule region to " << *I << "\n");
     return true;
   }
   // Search up and down at the same time, because we don't know if the new
   // instruction is above or below the existing scheduling region.
LLVM_DEBUG(dbgs() << "SLP:  extend schedule region start to " << *I
                       << "\n");
     return true;
   }
   assert((UpIter == UpperEnd || (DownIter != LowerEnd && &*DownIter == I)) &&
          "Expected to reach top of the basic block or instruction down the "
          "lower end.");
   assert(I->getParent() == ScheduleEnd->getParent() &&
          "Instruction is in wrong basic block.");
-  initScheduleData(ScheduleEnd, I->getNextNode(), LastLoadStoreInRegion,
+  initScheduleData(ScheduleEnd, I->getNextNonDebugInstruction(), LastLoadStoreInRegion,
                    nullptr);
-  ScheduleEnd = I->getNextNode();
+  ScheduleEnd = I->getNextNonDebugInstruction();
   if (isOneOf(S, I) != I)
     CheckScheduleForI(I);
   assert(ScheduleEnd && "tried to vectorize a terminator?");
   LLVM_DEBUG(dbgs() << "SLP:  extend schedule region end to " << *I << "\n");
   return true;
 }
 
 void BoUpSLP::BlockScheduling::initScheduleData(Instruction *FromI,
                                                 Instruction *ToI,
                                                 ScheduleData *PrevLoadStore,
                                                 ScheduleData *NextLoadStore) {
   ScheduleData *CurrentLoadStore = PrevLoadStore;
-  for (Instruction *I = FromI; I != ToI; I = I->getNextNode()) {
+  for (Instruction *I = FromI; I != ToI; I = I->getNextNonDebugInstruction()) {
     // No need to allocate data for non-schedulable instructions.
     if (doesNotNeedToBeScheduled(I))
       continue;
     ScheduleData *SD = ScheduleDataMap.lookup(I);
     if (!SD) {
       SD = allocateScheduleDataChunks();
       ScheduleDataMap[I] = SD;
       SD->Inst = I;
for (ScheduleData *BundleMember = SD; BundleMember;
         if (!DestBundle->hasValidDependencies())
           WorkList.push_back(DestBundle);
       };
 
       // Any instruction which isn't safe to speculate at the beginning of the
       // block is control dependend on any early exit or non-willreturn call
       // which proceeds it.
       if (!isGuaranteedToTransferExecutionToSuccessor(BundleMember->Inst)) {
-        for (Instruction *I = BundleMember->Inst->getNextNode();
-             I != ScheduleEnd; I = I->getNextNode()) {
+        for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+             I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
           if (isSafeToSpeculativelyExecute(I, &*BB->begin(), SLP->AC))
             continue;
 
           // Add the dependency
           makeControlDependent(I);
 
           if (!isGuaranteedToTransferExecutionToSuccessor(I))
             // Everything past here must be control dependent on I.
             break;
         }
       }
 
       if (RegionHasStackSave) {
         // If we have an inalloc alloca instruction, it needs to be scheduled
         // after any preceeding stacksave.  We also need to prevent any alloca
         // from reordering above a preceeding stackrestore.
         if (match(BundleMember->Inst, m_Intrinsic<Intrinsic::stacksave>()) ||
             match(BundleMember->Inst, m_Intrinsic<Intrinsic::stackrestore>())) {
-          for (Instruction *I = BundleMember->Inst->getNextNode();
-               I != ScheduleEnd; I = I->getNextNode()) {
+          for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+               I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
             if (match(I, m_Intrinsic<Intrinsic::stacksave>()) ||
                 match(I, m_Intrinsic<Intrinsic::stackrestore>()))
               // Any allocas past here must be control dependent on I, and I
               // must be memory dependend on BundleMember->Inst.
               break;
 
             if (!isa<AllocaInst>(I))
               continue;
 
             // Add the dependency
             makeControlDependent(I);
           }
         }
 
         // In addition to the cases handle just above, we need to prevent
         // allocas and loads/stores from moving below a stacksave or a
         // stackrestore. Avoiding moving allocas below stackrestore is currently
         // thought to be conservatism. Moving loads/stores below a stackrestore
         // can lead to incorrect code.
         if (isa<AllocaInst>(BundleMember->Inst) ||
             BundleMember->Inst->mayReadOrWriteMemory()) {
-          for (Instruction *I = BundleMember->Inst->getNextNode();
-               I != ScheduleEnd; I = I->getNextNode()) {
+          for (Instruction *I = BundleMember->Inst->getNextNonDebugInstruction();
+               I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
             if (!match(I, m_Intrinsic<Intrinsic::stacksave>()) &&
                 !match(I, m_Intrinsic<Intrinsic::stackrestore>()))
               continue;
 
             // Add the dependency
             makeControlDependent(I);
             break;
           }
if (InsertInReadyList && SD->isReady()) {
                         << "\n");
     }
   }
 }
 
 void BoUpSLP::BlockScheduling::resetSchedule() {
   assert(ScheduleStart &&
          "tried to reset schedule on block which has not been scheduled");
-  for (Instruction *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNode()) {
+  for (Instruction *I = ScheduleStart; I != ScheduleEnd; I = I->getNextNonDebugInstruction()) {
     doForAllOpcodes(I, [&](ScheduleData *SD) {
       assert(isInSchedulingRegion(SD) &&
              "ScheduleData not in scheduling region");
       SD->IsScheduled = false;
       SD->resetUnscheduledDeps();
     });
   }
   ReadyInsts.clear();
struct ScheduleDataCompare {
     }
   };
   std::set<ScheduleData *, ScheduleDataCompare> ReadyInsts;
 
   // Ensure that all dependency data is updated (for nodes in the sub-graph)
   // and fill the ready-list with initial instructions.
   int Idx = 0;
   for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd;
-       I = I->getNextNode()) {
+       I = I->getNextNonDebugInstruction()) {
     BS->doForAllOpcodes(I, [this, &Idx, BS](ScheduleData *SD) {
       TreeEntry *SDTE = getTreeEntry(SD->Inst);
       (void)SDTE;
       assert((isVectorLikeInstWithConstOps(SD->Inst) ||
               SD->isPartOfBundle() ==
                   (SDTE && !doesNotNeedToSchedule(SDTE->Scalars))) &&
              "scheduler and vectorizer bundle mismatch");
       SD->FirstInBundle->SchedulingPriority = Idx++;
while (!ReadyInsts.empty()) {
     ScheduleData *picked = *ReadyInsts.begin();
     ReadyInsts.erase(ReadyInsts.begin());
 
     // Move the scheduled instruction(s) to their dedicated places, if not
     // there yet.
     for (ScheduleData *BundleMember = picked; BundleMember;
          BundleMember = BundleMember->NextInBundle) {
       Instruction *pickedInst = BundleMember->Inst;
-      if (pickedInst->getNextNode() != LastScheduledInst)
+      if (pickedInst->getNextNonDebugInstruction() != LastScheduledInst)
         pickedInst->moveBefore(LastScheduledInst);
       LastScheduledInst = pickedInst;
     }
 
     BS->schedule(picked, ReadyInsts);
   }
 
   // Check that we didn't break any of our invariants.
 #ifdef EXPENSIVE_CHECKS
   BS->verify();
 #endif
 
 #if !defined(NDEBUG) || defined(EXPENSIVE_CHECKS)
   // Check that all schedulable entities got scheduled
-  for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd; I = I->getNextNode()) {
+  for (auto *I = BS->ScheduleStart; I != BS->ScheduleEnd; I = I->getNextNonDebugInstruction()) {
     BS->doForAllOpcodes(I, [&](ScheduleData *SD) {
       if (SD->isSchedulingEntity() && SD->hasValidDependencies()) {
         assert(SD->IsScheduled && "must be scheduled at this point");
       }
     });
   }
 #endif