[llvm] [DebugInfo][RemoveDIs] Have getInsertionPtAfterDef return an iterator (PR #73149)

Jeremy Morse via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 29 08:22:00 PST 2023


https://github.com/jmorse updated https://github.com/llvm/llvm-project/pull/73149

>From 2ee48d5211e4aac945bd0230f77b0103f667088d Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Thu, 22 Jun 2023 11:14:23 +0100
Subject: [PATCH 1/3] [DebugInfo][RemoveDIs] Have getInsertionPtAfterDef return
 an iterator

Part of the "RemoveDIs" project to remove debug intrinsics requires passing
block-positions around in iterators rather than as instruction pointers,
allowing some debug-info to reside in BasicBlock::iterator. This means
getInsertionPointAfterDef has to return an iterator, and as it can return
no-instruction that means returning an optional iterator.

This patch changes the signature for getInsertionPtAfterDef and then
patches up the various places that use it to handle the different type.
This would overall be an NFC patch, however in
InstCombinerImpl::freezeOtherUses I've started skipping any debug
intrinsics at the returned insert-position. This should not have any
_meaningful_ effect on the compiler output: at worst it means variable
assignments that are skipped will now cover the freeze instruction and
anything inserted before it, which should be inconsequential.
---
 llvm/include/llvm/IR/IRBuilder.h              |  8 +++
 llvm/include/llvm/IR/Instruction.h            |  4 +-
 llvm/lib/IR/Instruction.cpp                   | 12 +++--
 llvm/lib/Transforms/Coroutines/CoroFrame.cpp  |  6 +--
 .../InstCombine/InstCombineAndOrXor.cpp       |  6 +--
 .../InstCombine/InstCombineCalls.cpp          |  6 +--
 .../InstCombine/InstructionCombining.cpp      | 20 +++++---
 llvm/lib/Transforms/Scalar/GuardWidening.cpp  | 49 ++++++++++++-------
 .../Transforms/Scalar/LoopIdiomRecognize.cpp  |  2 +-
 llvm/lib/Transforms/Scalar/Reassociate.cpp    | 14 ++++--
 .../Transforms/Vectorize/VectorCombine.cpp    |  8 +--
 11 files changed, 88 insertions(+), 47 deletions(-)

diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
index e3c4e76f90a4cfc..2cd9a665e9e5d05 100644
--- a/llvm/include/llvm/IR/IRBuilder.h
+++ b/llvm/include/llvm/IR/IRBuilder.h
@@ -200,6 +200,14 @@ class IRBuilderBase {
       SetCurrentDebugLocation(IP->getStableDebugLoc());
   }
 
+  /// This specifies that created instructions should be inserted at
+  /// the specified point, but also requires that \p IP is dereferencable.
+  void SetInsertPoint(BasicBlock::iterator IP) {
+    BB = IP->getParent();
+    InsertPt = IP;
+    SetCurrentDebugLocation(IP->getStableDebugLoc());
+  }
+
   /// This specifies that created instructions should inserted at the beginning
   /// end of the specified function, but after already existing static alloca
   /// instructions that are at the start.
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 58fc32237367d93..48fec7dde468a83 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -224,7 +224,9 @@ class Instruction : public User,
   /// of cases, e.g. phi nodes or terminators that return values. This function
   /// may return null if the insertion after the definition is not possible,
   /// e.g. due to a catchswitch terminator.
-  Instruction *getInsertionPointAfterDef();
+  std::optional<
+      SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator>
+  getInsertionPointAfterDef();
 
   //===--------------------------------------------------------------------===//
   // Subclass classification.
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 7449692f05d7bf9..e4b3e911a71f19b 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -274,7 +274,7 @@ bool Instruction::comesBefore(const Instruction *Other) const {
   return Order < Other->Order;
 }
 
-Instruction *Instruction::getInsertionPointAfterDef() {
+std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
   assert(!getType()->isVoidTy() && "Instruction must define result");
   BasicBlock *InsertBB;
   BasicBlock::iterator InsertPt;
@@ -287,18 +287,22 @@ Instruction *Instruction::getInsertionPointAfterDef() {
   } else if (isa<CallBrInst>(this)) {
     // Def is available in multiple successors, there's no single dominating
     // insertion point.
-    return nullptr;
+    return std::nullopt;
   } else {
     assert(!isTerminator() && "Only invoke/callbr terminators return value");
     InsertBB = getParent();
     InsertPt = std::next(getIterator());
+    // Any instruction inserted immediately after "this" will come before any
+    // debug-info records take effect -- thus, set the head bit indicating that
+    // to debug-info-transfer code.
+    InsertPt.setHeadBit(true);
   }
 
   // catchswitch blocks don't have any legal insertion point (because they
   // are both an exception pad and a terminator).
   if (InsertPt == InsertBB->end())
-    return nullptr;
-  return &*InsertPt;
+    return std::nullopt;
+  return InsertPt;
 }
 
 bool Instruction::isOnlyUserOfAnyOperand() {
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index fef1a698e146390..1134b20880f1830 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -2886,13 +2886,13 @@ void coro::salvageDebugInfo(
   // dbg.value since it does not have the same function wide guarantees that
   // dbg.declare does.
   if (isa<DbgDeclareInst>(DVI)) {
-    Instruction *InsertPt = nullptr;
+    std::optional<BasicBlock::iterator> InsertPt;
     if (auto *I = dyn_cast<Instruction>(Storage))
       InsertPt = I->getInsertionPointAfterDef();
     else if (isa<Argument>(Storage))
-      InsertPt = &*F->getEntryBlock().begin();
+      InsertPt = F->getEntryBlock().begin();
     if (InsertPt)
-      DVI->moveBefore(InsertPt);
+      DVI->moveBefore(*(*InsertPt)->getParent(), *InsertPt);
   }
 }
 
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 02881109f17d29f..557447a352576a6 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -4130,7 +4130,7 @@ static bool canFreelyInvert(InstCombiner &IC, Value *Op,
 static Value *freelyInvert(InstCombinerImpl &IC, Value *Op,
                            Instruction *IgnoredUser) {
   auto *I = cast<Instruction>(Op);
-  IC.Builder.SetInsertPoint(&*I->getInsertionPointAfterDef());
+  IC.Builder.SetInsertPoint(&**I->getInsertionPointAfterDef());
   Value *NotOp = IC.Builder.CreateNot(Op, Op->getName() + ".not");
   Op->replaceUsesWithIf(NotOp,
                         [NotOp](Use &U) { return U.getUser() != NotOp; });
@@ -4168,7 +4168,7 @@ bool InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
   Op0 = freelyInvert(*this, Op0, &I);
   Op1 = freelyInvert(*this, Op1, &I);
 
-  Builder.SetInsertPoint(I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
   Value *NewLogicOp;
   if (IsBinaryOp)
     NewLogicOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
@@ -4216,7 +4216,7 @@ bool InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
 
   *OpToInvert = freelyInvert(*this, *OpToInvert, &I);
 
-  Builder.SetInsertPoint(&*I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
   Value *NewBinOp;
   if (IsBinaryOp)
     NewBinOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index f8346cd03849acf..a991f0906052a30 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -4017,9 +4017,9 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
       NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);
       NC->setDebugLoc(Caller->getDebugLoc());
 
-      Instruction *InsertPt = NewCall->getInsertionPointAfterDef();
-      assert(InsertPt && "No place to insert cast");
-      InsertNewInstBefore(NC, InsertPt->getIterator());
+      auto OptInsertPt = NewCall->getInsertionPointAfterDef();
+      assert(OptInsertPt && "No place to insert cast");
+      InsertNewInstBefore(NC, *OptInsertPt);
       Worklist.pushUsersToWorkList(*Caller);
     } else {
       NV = PoisonValue::get(Caller->getType());
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index e6088541349784b..597378605125129 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3807,19 +3807,27 @@ bool InstCombinerImpl::freezeOtherUses(FreezeInst &FI) {
   // *all* uses if the operand is an invoke/callbr and the use is in a phi on
   // the normal/default destination. This is why the domination check in the
   // replacement below is still necessary.
-  Instruction *MoveBefore;
+  BasicBlock::iterator MoveBefore;
   if (isa<Argument>(Op)) {
     MoveBefore =
-        &*FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
+        FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
   } else {
-    MoveBefore = cast<Instruction>(Op)->getInsertionPointAfterDef();
-    if (!MoveBefore)
+    auto MoveBeforeOpt = cast<Instruction>(Op)->getInsertionPointAfterDef();
+    if (!MoveBeforeOpt)
       return false;
+    MoveBefore = *MoveBeforeOpt;
   }
 
+  // Don't move to the position of a debug intrinsic.
+  if (isa<DbgInfoIntrinsic>(MoveBefore))
+    MoveBefore = MoveBefore->getNextNonDebugInstruction()->getIterator();
+  // Re-point iterator to come after any debug-info records, if we're
+  // running in "RemoveDIs" mode
+  MoveBefore.setHeadBit(false);
+
   bool Changed = false;
-  if (&FI != MoveBefore) {
-    FI.moveBefore(*MoveBefore->getParent(), MoveBefore->getIterator());
+  if (&FI != &*MoveBefore) {
+    FI.moveBefore(*MoveBefore->getParent(), MoveBefore);
     Changed = true;
   }
 
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index 5506dd82efd4557..61d2912ff70fce1 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -596,35 +596,47 @@ void GuardWideningImpl::makeAvailableAt(Value *V, Instruction *Loc) const {
 }
 
 // Return Instruction before which we can insert freeze for the value V as close
-// to def as possible. If there is no place to add freeze, return nullptr.
-static Instruction *getFreezeInsertPt(Value *V, const DominatorTree &DT) {
+// to def as possible. If there is no place to add freeze, return empty.
+static std::optional<BasicBlock::iterator>
+getFreezeInsertPt(Value *V, const DominatorTree &DT) {
   auto *I = dyn_cast<Instruction>(V);
   if (!I)
-    return &*DT.getRoot()->getFirstNonPHIOrDbgOrAlloca();
+    return DT.getRoot()->getFirstNonPHIOrDbgOrAlloca()->getIterator();
 
-  auto *Res = I->getInsertionPointAfterDef();
+  std::optional<BasicBlock::iterator> Res = I->getInsertionPointAfterDef();
   // If there is no place to add freeze - return nullptr.
-  if (!Res || !DT.dominates(I, Res))
-    return nullptr;
+  if (!Res || !DT.dominates(I, &**Res))
+    return std::nullopt;
+
+  Instruction *ResInst = &**Res;
 
   // If there is a User dominated by original I, then it should be dominated
   // by Freeze instruction as well.
   if (any_of(I->users(), [&](User *U) {
         Instruction *User = cast<Instruction>(U);
-        return Res != User && DT.dominates(I, User) && !DT.dominates(Res, User);
+        return ResInst != User && DT.dominates(I, User) &&
+               !DT.dominates(ResInst, User);
       }))
-    return nullptr;
+    return std::nullopt;
   return Res;
 }
 
 Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
   if (isGuaranteedNotToBePoison(Orig, nullptr, InsertPt, &DT))
     return Orig;
-  Instruction *InsertPtAtDef = getFreezeInsertPt(Orig, DT);
-  if (!InsertPtAtDef)
-    return new FreezeInst(Orig, "gw.freeze", InsertPt);
-  if (isa<Constant>(Orig) || isa<GlobalValue>(Orig))
-    return new FreezeInst(Orig, "gw.freeze", InsertPtAtDef);
+  std::optional<BasicBlock::iterator> InsertPtAtDef =
+      getFreezeInsertPt(Orig, DT);
+  if (!InsertPtAtDef) {
+    FreezeInst *FI = new FreezeInst(Orig, "gw.freeze");
+    FI->insertBefore(InsertPt);
+    return FI;
+  }
+  if (isa<Constant>(Orig) || isa<GlobalValue>(Orig)) {
+    BasicBlock::iterator InsertPt = *InsertPtAtDef;
+    FreezeInst *FI = new FreezeInst(Orig, "gw.freeze");
+    FI->insertBefore(*InsertPt->getParent(), InsertPt);
+    return FI;
+  }
 
   SmallSet<Value *, 16> Visited;
   SmallVector<Value *, 16> Worklist;
@@ -643,8 +655,10 @@ Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
     if (Visited.insert(Def).second) {
       if (isGuaranteedNotToBePoison(Def, nullptr, InsertPt, &DT))
         return true;
-      CacheOfFreezes[Def] = new FreezeInst(Def, Def->getName() + ".gw.fr",
-                                           getFreezeInsertPt(Def, DT));
+      BasicBlock::iterator InsertPt = *getFreezeInsertPt(Def, DT);
+      FreezeInst *FI = new FreezeInst(Def, Def->getName() + ".gw.fr");
+      FI->insertBefore(*InsertPt->getParent(), InsertPt);
+      CacheOfFreezes[Def] = FI;
     }
 
     if (CacheOfFreezes.count(Def))
@@ -685,8 +699,9 @@ Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
 
   Value *Result = Orig;
   for (Value *V : NeedFreeze) {
-    auto *FreezeInsertPt = getFreezeInsertPt(V, DT);
-    FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr", FreezeInsertPt);
+    auto FreezeInsertPt = *getFreezeInsertPt(V, DT);
+    FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr");
+    FI->insertBefore(*FreezeInsertPt->getParent(), FreezeInsertPt);
     ++FreezeAdded;
     if (V == Orig)
       Result = FI;
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 4093a5a51a4d79e..095ffda7fff1eaa 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -2411,7 +2411,7 @@ bool LoopIdiomRecognize::recognizeShiftUntilBitTest() {
     // it's use count.
     Instruction *InsertPt = nullptr;
     if (auto *BitPosI = dyn_cast<Instruction>(BitPos))
-      InsertPt = BitPosI->getInsertionPointAfterDef();
+      InsertPt = &**BitPosI->getInsertionPointAfterDef();
     else
       InsertPt = &*DT->getRoot()->getFirstNonPHIOrDbgOrAlloca();
     if (!InsertPt)
diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index 9c4a344d4295f8a..0d55c72e407e928 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -921,16 +921,20 @@ static Value *NegateValue(Value *V, Instruction *BI,
         TheNeg->getParent()->getParent() != BI->getParent()->getParent())
       continue;
 
-    Instruction *InsertPt;
+    BasicBlock::iterator InsertPt;
     if (Instruction *InstInput = dyn_cast<Instruction>(V)) {
-      InsertPt = InstInput->getInsertionPointAfterDef();
-      if (!InsertPt)
+      auto InsertPtOpt = InstInput->getInsertionPointAfterDef();
+      if (!InsertPtOpt)
         continue;
+      InsertPt = *InsertPtOpt;
     } else {
-      InsertPt = &*TheNeg->getFunction()->getEntryBlock().begin();
+      InsertPt = TheNeg->getFunction()
+                     ->getEntryBlock()
+                     .getFirstNonPHIOrDbg()
+                     ->getIterator();
     }
 
-    TheNeg->moveBefore(InsertPt);
+    TheNeg->moveBefore(*InsertPt->getParent(), InsertPt);
     if (TheNeg->getOpcode() == Instruction::Sub) {
       TheNeg->setHasNoUnsignedWrap(false);
       TheNeg->setHasNoSignedWrap(false);
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index acf2b0e154e8267..f3cf6fbf427bea7 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1810,16 +1810,16 @@ bool VectorCombine::foldSelectShuffle(Instruction &I, bool FromReduction) {
           return SSV->getOperand(Op);
     return SV->getOperand(Op);
   };
-  Builder.SetInsertPoint(SVI0A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI0A->getInsertionPointAfterDef());
   Value *NSV0A = Builder.CreateShuffleVector(GetShuffleOperand(SVI0A, 0),
                                              GetShuffleOperand(SVI0A, 1), V1A);
-  Builder.SetInsertPoint(SVI0B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI0B->getInsertionPointAfterDef());
   Value *NSV0B = Builder.CreateShuffleVector(GetShuffleOperand(SVI0B, 0),
                                              GetShuffleOperand(SVI0B, 1), V1B);
-  Builder.SetInsertPoint(SVI1A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI1A->getInsertionPointAfterDef());
   Value *NSV1A = Builder.CreateShuffleVector(GetShuffleOperand(SVI1A, 0),
                                              GetShuffleOperand(SVI1A, 1), V2A);
-  Builder.SetInsertPoint(SVI1B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI1B->getInsertionPointAfterDef());
   Value *NSV1B = Builder.CreateShuffleVector(GetShuffleOperand(SVI1B, 0),
                                              GetShuffleOperand(SVI1B, 1), V2B);
   Builder.SetInsertPoint(Op0);

>From 77ff164c533eed4375fc4928263092c9ed0ab0ca Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Fri, 24 Nov 2023 10:15:29 +0000
Subject: [PATCH 2/3] Experiment with returning end() from
 getInsertionPointAfterDef

Rather than returning std::nullopt.
---
 llvm/include/llvm/IR/Instruction.h              |  7 +++----
 llvm/lib/IR/Instruction.cpp                     | 16 ++++++----------
 llvm/lib/Transforms/Coroutines/CoroFrame.cpp    | 17 ++++++++++++-----
 .../InstCombine/InstCombineAndOrXor.cpp         |  6 +++---
 .../Transforms/InstCombine/InstCombineCalls.cpp |  7 ++++---
 .../InstCombine/InstructionCombining.cpp        |  6 +++---
 llvm/lib/Transforms/Scalar/GuardWidening.cpp    |  6 +++---
 .../Transforms/Scalar/LoopIdiomRecognize.cpp    |  2 +-
 llvm/lib/Transforms/Scalar/Reassociate.cpp      |  5 ++---
 llvm/lib/Transforms/Vectorize/VectorCombine.cpp |  8 ++++----
 10 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 48fec7dde468a83..82cf2e8ef338c2c 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -222,10 +222,9 @@ class Instruction : public User,
   /// Get the first insertion point at which the result of this instruction
   /// is defined. This is *not* the directly following instruction in a number
   /// of cases, e.g. phi nodes or terminators that return values. This function
-  /// may return null if the insertion after the definition is not possible,
-  /// e.g. due to a catchswitch terminator.
-  std::optional<
-      SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator>
+  /// may return getParent()->end() if the insertion after the definition is not
+  /// possible, e.g. due to a catchswitch terminator.
+  SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator
   getInsertionPointAfterDef();
 
   //===--------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index e4b3e911a71f19b..4ea404aafbeccb5 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -274,23 +274,20 @@ bool Instruction::comesBefore(const Instruction *Other) const {
   return Order < Other->Order;
 }
 
-std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
+BasicBlock::iterator Instruction::getInsertionPointAfterDef() {
   assert(!getType()->isVoidTy() && "Instruction must define result");
-  BasicBlock *InsertBB;
   BasicBlock::iterator InsertPt;
   if (auto *PN = dyn_cast<PHINode>(this)) {
-    InsertBB = PN->getParent();
-    InsertPt = InsertBB->getFirstInsertionPt();
+    InsertPt = getParent()->getFirstInsertionPt();
   } else if (auto *II = dyn_cast<InvokeInst>(this)) {
-    InsertBB = II->getNormalDest();
+    BasicBlock *InsertBB = II->getNormalDest();
     InsertPt = InsertBB->getFirstInsertionPt();
   } else if (isa<CallBrInst>(this)) {
     // Def is available in multiple successors, there's no single dominating
     // insertion point.
-    return std::nullopt;
+    return getParent()->end();
   } else {
     assert(!isTerminator() && "Only invoke/callbr terminators return value");
-    InsertBB = getParent();
     InsertPt = std::next(getIterator());
     // Any instruction inserted immediately after "this" will come before any
     // debug-info records take effect -- thus, set the head bit indicating that
@@ -299,9 +296,8 @@ std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
   }
 
   // catchswitch blocks don't have any legal insertion point (because they
-  // are both an exception pad and a terminator).
-  if (InsertPt == InsertBB->end())
-    return std::nullopt;
+  // are both an exception pad and a terminator), and will cause end() to be
+  // returned here.
   return InsertPt;
 }
 
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 1134b20880f1830..cdb15f63a11f7e6 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -2886,13 +2886,20 @@ void coro::salvageDebugInfo(
   // dbg.value since it does not have the same function wide guarantees that
   // dbg.declare does.
   if (isa<DbgDeclareInst>(DVI)) {
-    std::optional<BasicBlock::iterator> InsertPt;
-    if (auto *I = dyn_cast<Instruction>(Storage))
+    BasicBlock::iterator InsertPt;
+    BasicBlock *InsertBB = nullptr;
+    if (auto *I = dyn_cast<Instruction>(Storage)) {
       InsertPt = I->getInsertionPointAfterDef();
-    else if (isa<Argument>(Storage))
+      // There might not be anywhere to insert.
+      if (InsertPt == I->getParent()->end())
+        return;
+      InsertBB = InsertPt->getParent();
+    } else if (isa<Argument>(Storage)) {
       InsertPt = F->getEntryBlock().begin();
-    if (InsertPt)
-      DVI->moveBefore(*(*InsertPt)->getParent(), *InsertPt);
+      InsertBB = &F->getEntryBlock();
+    } else
+      return;
+    DVI->moveBefore(*InsertBB, InsertPt);
   }
 }
 
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 557447a352576a6..594a83d2c0e9c4d 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -4130,7 +4130,7 @@ static bool canFreelyInvert(InstCombiner &IC, Value *Op,
 static Value *freelyInvert(InstCombinerImpl &IC, Value *Op,
                            Instruction *IgnoredUser) {
   auto *I = cast<Instruction>(Op);
-  IC.Builder.SetInsertPoint(&**I->getInsertionPointAfterDef());
+  IC.Builder.SetInsertPoint(&*I->getInsertionPointAfterDef());
   Value *NotOp = IC.Builder.CreateNot(Op, Op->getName() + ".not");
   Op->replaceUsesWithIf(NotOp,
                         [NotOp](Use &U) { return U.getUser() != NotOp; });
@@ -4168,7 +4168,7 @@ bool InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
   Op0 = freelyInvert(*this, Op0, &I);
   Op1 = freelyInvert(*this, Op1, &I);
 
-  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(I.getInsertionPointAfterDef());
   Value *NewLogicOp;
   if (IsBinaryOp)
     NewLogicOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
@@ -4216,7 +4216,7 @@ bool InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
 
   *OpToInvert = freelyInvert(*this, *OpToInvert, &I);
 
-  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(I.getInsertionPointAfterDef());
   Value *NewBinOp;
   if (IsBinaryOp)
     NewBinOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index a991f0906052a30..b6198326514ea36 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -4017,9 +4017,10 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
       NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);
       NC->setDebugLoc(Caller->getDebugLoc());
 
-      auto OptInsertPt = NewCall->getInsertionPointAfterDef();
-      assert(OptInsertPt && "No place to insert cast");
-      InsertNewInstBefore(NC, *OptInsertPt);
+      auto InsertPt = NewCall->getInsertionPointAfterDef();
+      assert(InsertPt != NewCall->getParent()->end() &&
+             "No place to insert cast");
+      InsertNewInstBefore(NC, InsertPt);
       Worklist.pushUsersToWorkList(*Caller);
     } else {
       NV = PoisonValue::get(Caller->getType());
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 597378605125129..d9efa01fb9b77af 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3812,10 +3812,10 @@ bool InstCombinerImpl::freezeOtherUses(FreezeInst &FI) {
     MoveBefore =
         FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
   } else {
-    auto MoveBeforeOpt = cast<Instruction>(Op)->getInsertionPointAfterDef();
-    if (!MoveBeforeOpt)
+    Instruction *IOp = cast<Instruction>(Op);
+    MoveBefore = IOp->getInsertionPointAfterDef();
+    if (MoveBefore == IOp->getParent()->end())
       return false;
-    MoveBefore = *MoveBeforeOpt;
   }
 
   // Don't move to the position of a debug intrinsic.
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index 61d2912ff70fce1..fd704c0db267869 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -603,12 +603,12 @@ getFreezeInsertPt(Value *V, const DominatorTree &DT) {
   if (!I)
     return DT.getRoot()->getFirstNonPHIOrDbgOrAlloca()->getIterator();
 
-  std::optional<BasicBlock::iterator> Res = I->getInsertionPointAfterDef();
+  BasicBlock::iterator Res = I->getInsertionPointAfterDef();
   // If there is no place to add freeze - return nullptr.
-  if (!Res || !DT.dominates(I, &**Res))
+  if (Res == I->getParent()->end() || !DT.dominates(I, &*Res))
     return std::nullopt;
 
-  Instruction *ResInst = &**Res;
+  Instruction *ResInst = &*Res;
 
   // If there is a User dominated by original I, then it should be dominated
   // by Freeze instruction as well.
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 095ffda7fff1eaa..2bc0c3efeac4c60 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -2411,7 +2411,7 @@ bool LoopIdiomRecognize::recognizeShiftUntilBitTest() {
     // it's use count.
     Instruction *InsertPt = nullptr;
     if (auto *BitPosI = dyn_cast<Instruction>(BitPos))
-      InsertPt = &**BitPosI->getInsertionPointAfterDef();
+      InsertPt = &*BitPosI->getInsertionPointAfterDef();
     else
       InsertPt = &*DT->getRoot()->getFirstNonPHIOrDbgOrAlloca();
     if (!InsertPt)
diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index 0d55c72e407e928..f0ccee5b0f4b011 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -923,10 +923,9 @@ static Value *NegateValue(Value *V, Instruction *BI,
 
     BasicBlock::iterator InsertPt;
     if (Instruction *InstInput = dyn_cast<Instruction>(V)) {
-      auto InsertPtOpt = InstInput->getInsertionPointAfterDef();
-      if (!InsertPtOpt)
+      InsertPt = InstInput->getInsertionPointAfterDef();
+      if (InsertPt == InstInput->getParent()->end())
         continue;
-      InsertPt = *InsertPtOpt;
     } else {
       InsertPt = TheNeg->getFunction()
                      ->getEntryBlock()
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index f3cf6fbf427bea7..acf2b0e154e8267 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1810,16 +1810,16 @@ bool VectorCombine::foldSelectShuffle(Instruction &I, bool FromReduction) {
           return SSV->getOperand(Op);
     return SV->getOperand(Op);
   };
-  Builder.SetInsertPoint(*SVI0A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(SVI0A->getInsertionPointAfterDef());
   Value *NSV0A = Builder.CreateShuffleVector(GetShuffleOperand(SVI0A, 0),
                                              GetShuffleOperand(SVI0A, 1), V1A);
-  Builder.SetInsertPoint(*SVI0B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(SVI0B->getInsertionPointAfterDef());
   Value *NSV0B = Builder.CreateShuffleVector(GetShuffleOperand(SVI0B, 0),
                                              GetShuffleOperand(SVI0B, 1), V1B);
-  Builder.SetInsertPoint(*SVI1A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(SVI1A->getInsertionPointAfterDef());
   Value *NSV1A = Builder.CreateShuffleVector(GetShuffleOperand(SVI1A, 0),
                                              GetShuffleOperand(SVI1A, 1), V2A);
-  Builder.SetInsertPoint(*SVI1B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(SVI1B->getInsertionPointAfterDef());
   Value *NSV1B = Builder.CreateShuffleVector(GetShuffleOperand(SVI1B, 0),
                                              GetShuffleOperand(SVI1B, 1), V2B);
   Builder.SetInsertPoint(Op0);

>From 5cf5db41d60283738d5220930754c0eed64bb027 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Wed, 29 Nov 2023 16:21:09 +0000
Subject: [PATCH 3/3] Revert "Experiment with returning end() from
 getInsertionPointAfterDef"

This reverts commit 77ff164c533eed4375fc4928263092c9ed0ab0ca.
---
 llvm/include/llvm/IR/Instruction.h              |  7 ++++---
 llvm/lib/IR/Instruction.cpp                     | 16 ++++++++++------
 llvm/lib/Transforms/Coroutines/CoroFrame.cpp    | 17 +++++------------
 .../InstCombine/InstCombineAndOrXor.cpp         |  6 +++---
 .../Transforms/InstCombine/InstCombineCalls.cpp |  7 +++----
 .../InstCombine/InstructionCombining.cpp        |  6 +++---
 llvm/lib/Transforms/Scalar/GuardWidening.cpp    |  6 +++---
 .../Transforms/Scalar/LoopIdiomRecognize.cpp    |  2 +-
 llvm/lib/Transforms/Scalar/Reassociate.cpp      |  5 +++--
 llvm/lib/Transforms/Vectorize/VectorCombine.cpp |  8 ++++----
 10 files changed, 39 insertions(+), 41 deletions(-)

diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 82cf2e8ef338c2c..48fec7dde468a83 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -222,9 +222,10 @@ class Instruction : public User,
   /// Get the first insertion point at which the result of this instruction
   /// is defined. This is *not* the directly following instruction in a number
   /// of cases, e.g. phi nodes or terminators that return values. This function
-  /// may return getParent()->end() if the insertion after the definition is not
-  /// possible, e.g. due to a catchswitch terminator.
-  SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator
+  /// may return null if the insertion after the definition is not possible,
+  /// e.g. due to a catchswitch terminator.
+  std::optional<
+      SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator>
   getInsertionPointAfterDef();
 
   //===--------------------------------------------------------------------===//
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 4ea404aafbeccb5..e4b3e911a71f19b 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -274,20 +274,23 @@ bool Instruction::comesBefore(const Instruction *Other) const {
   return Order < Other->Order;
 }
 
-BasicBlock::iterator Instruction::getInsertionPointAfterDef() {
+std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
   assert(!getType()->isVoidTy() && "Instruction must define result");
+  BasicBlock *InsertBB;
   BasicBlock::iterator InsertPt;
   if (auto *PN = dyn_cast<PHINode>(this)) {
-    InsertPt = getParent()->getFirstInsertionPt();
+    InsertBB = PN->getParent();
+    InsertPt = InsertBB->getFirstInsertionPt();
   } else if (auto *II = dyn_cast<InvokeInst>(this)) {
-    BasicBlock *InsertBB = II->getNormalDest();
+    InsertBB = II->getNormalDest();
     InsertPt = InsertBB->getFirstInsertionPt();
   } else if (isa<CallBrInst>(this)) {
     // Def is available in multiple successors, there's no single dominating
     // insertion point.
-    return getParent()->end();
+    return std::nullopt;
   } else {
     assert(!isTerminator() && "Only invoke/callbr terminators return value");
+    InsertBB = getParent();
     InsertPt = std::next(getIterator());
     // Any instruction inserted immediately after "this" will come before any
     // debug-info records take effect -- thus, set the head bit indicating that
@@ -296,8 +299,9 @@ BasicBlock::iterator Instruction::getInsertionPointAfterDef() {
   }
 
   // catchswitch blocks don't have any legal insertion point (because they
-  // are both an exception pad and a terminator), and will cause end() to be
-  // returned here.
+  // are both an exception pad and a terminator).
+  if (InsertPt == InsertBB->end())
+    return std::nullopt;
   return InsertPt;
 }
 
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index cdb15f63a11f7e6..1134b20880f1830 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -2886,20 +2886,13 @@ void coro::salvageDebugInfo(
   // dbg.value since it does not have the same function wide guarantees that
   // dbg.declare does.
   if (isa<DbgDeclareInst>(DVI)) {
-    BasicBlock::iterator InsertPt;
-    BasicBlock *InsertBB = nullptr;
-    if (auto *I = dyn_cast<Instruction>(Storage)) {
+    std::optional<BasicBlock::iterator> InsertPt;
+    if (auto *I = dyn_cast<Instruction>(Storage))
       InsertPt = I->getInsertionPointAfterDef();
-      // There might not be anywhere to insert.
-      if (InsertPt == I->getParent()->end())
-        return;
-      InsertBB = InsertPt->getParent();
-    } else if (isa<Argument>(Storage)) {
+    else if (isa<Argument>(Storage))
       InsertPt = F->getEntryBlock().begin();
-      InsertBB = &F->getEntryBlock();
-    } else
-      return;
-    DVI->moveBefore(*InsertBB, InsertPt);
+    if (InsertPt)
+      DVI->moveBefore(*(*InsertPt)->getParent(), *InsertPt);
   }
 }
 
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 594a83d2c0e9c4d..557447a352576a6 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -4130,7 +4130,7 @@ static bool canFreelyInvert(InstCombiner &IC, Value *Op,
 static Value *freelyInvert(InstCombinerImpl &IC, Value *Op,
                            Instruction *IgnoredUser) {
   auto *I = cast<Instruction>(Op);
-  IC.Builder.SetInsertPoint(&*I->getInsertionPointAfterDef());
+  IC.Builder.SetInsertPoint(&**I->getInsertionPointAfterDef());
   Value *NotOp = IC.Builder.CreateNot(Op, Op->getName() + ".not");
   Op->replaceUsesWithIf(NotOp,
                         [NotOp](Use &U) { return U.getUser() != NotOp; });
@@ -4168,7 +4168,7 @@ bool InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
   Op0 = freelyInvert(*this, Op0, &I);
   Op1 = freelyInvert(*this, Op1, &I);
 
-  Builder.SetInsertPoint(I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
   Value *NewLogicOp;
   if (IsBinaryOp)
     NewLogicOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
@@ -4216,7 +4216,7 @@ bool InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
 
   *OpToInvert = freelyInvert(*this, *OpToInvert, &I);
 
-  Builder.SetInsertPoint(I.getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
   Value *NewBinOp;
   if (IsBinaryOp)
     NewBinOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index b6198326514ea36..a991f0906052a30 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -4017,10 +4017,9 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
       NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);
       NC->setDebugLoc(Caller->getDebugLoc());
 
-      auto InsertPt = NewCall->getInsertionPointAfterDef();
-      assert(InsertPt != NewCall->getParent()->end() &&
-             "No place to insert cast");
-      InsertNewInstBefore(NC, InsertPt);
+      auto OptInsertPt = NewCall->getInsertionPointAfterDef();
+      assert(OptInsertPt && "No place to insert cast");
+      InsertNewInstBefore(NC, *OptInsertPt);
       Worklist.pushUsersToWorkList(*Caller);
     } else {
       NV = PoisonValue::get(Caller->getType());
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index d9efa01fb9b77af..597378605125129 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3812,10 +3812,10 @@ bool InstCombinerImpl::freezeOtherUses(FreezeInst &FI) {
     MoveBefore =
         FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
   } else {
-    Instruction *IOp = cast<Instruction>(Op);
-    MoveBefore = IOp->getInsertionPointAfterDef();
-    if (MoveBefore == IOp->getParent()->end())
+    auto MoveBeforeOpt = cast<Instruction>(Op)->getInsertionPointAfterDef();
+    if (!MoveBeforeOpt)
       return false;
+    MoveBefore = *MoveBeforeOpt;
   }
 
   // Don't move to the position of a debug intrinsic.
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index fd704c0db267869..61d2912ff70fce1 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -603,12 +603,12 @@ getFreezeInsertPt(Value *V, const DominatorTree &DT) {
   if (!I)
     return DT.getRoot()->getFirstNonPHIOrDbgOrAlloca()->getIterator();
 
-  BasicBlock::iterator Res = I->getInsertionPointAfterDef();
+  std::optional<BasicBlock::iterator> Res = I->getInsertionPointAfterDef();
   // If there is no place to add freeze - return nullptr.
-  if (Res == I->getParent()->end() || !DT.dominates(I, &*Res))
+  if (!Res || !DT.dominates(I, &**Res))
     return std::nullopt;
 
-  Instruction *ResInst = &*Res;
+  Instruction *ResInst = &**Res;
 
   // If there is a User dominated by original I, then it should be dominated
   // by Freeze instruction as well.
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 2bc0c3efeac4c60..095ffda7fff1eaa 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -2411,7 +2411,7 @@ bool LoopIdiomRecognize::recognizeShiftUntilBitTest() {
     // it's use count.
     Instruction *InsertPt = nullptr;
     if (auto *BitPosI = dyn_cast<Instruction>(BitPos))
-      InsertPt = &*BitPosI->getInsertionPointAfterDef();
+      InsertPt = &**BitPosI->getInsertionPointAfterDef();
     else
       InsertPt = &*DT->getRoot()->getFirstNonPHIOrDbgOrAlloca();
     if (!InsertPt)
diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index f0ccee5b0f4b011..0d55c72e407e928 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -923,9 +923,10 @@ static Value *NegateValue(Value *V, Instruction *BI,
 
     BasicBlock::iterator InsertPt;
     if (Instruction *InstInput = dyn_cast<Instruction>(V)) {
-      InsertPt = InstInput->getInsertionPointAfterDef();
-      if (InsertPt == InstInput->getParent()->end())
+      auto InsertPtOpt = InstInput->getInsertionPointAfterDef();
+      if (!InsertPtOpt)
         continue;
+      InsertPt = *InsertPtOpt;
     } else {
       InsertPt = TheNeg->getFunction()
                      ->getEntryBlock()
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index acf2b0e154e8267..f3cf6fbf427bea7 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1810,16 +1810,16 @@ bool VectorCombine::foldSelectShuffle(Instruction &I, bool FromReduction) {
           return SSV->getOperand(Op);
     return SV->getOperand(Op);
   };
-  Builder.SetInsertPoint(SVI0A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI0A->getInsertionPointAfterDef());
   Value *NSV0A = Builder.CreateShuffleVector(GetShuffleOperand(SVI0A, 0),
                                              GetShuffleOperand(SVI0A, 1), V1A);
-  Builder.SetInsertPoint(SVI0B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI0B->getInsertionPointAfterDef());
   Value *NSV0B = Builder.CreateShuffleVector(GetShuffleOperand(SVI0B, 0),
                                              GetShuffleOperand(SVI0B, 1), V1B);
-  Builder.SetInsertPoint(SVI1A->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI1A->getInsertionPointAfterDef());
   Value *NSV1A = Builder.CreateShuffleVector(GetShuffleOperand(SVI1A, 0),
                                              GetShuffleOperand(SVI1A, 1), V2A);
-  Builder.SetInsertPoint(SVI1B->getInsertionPointAfterDef());
+  Builder.SetInsertPoint(*SVI1B->getInsertionPointAfterDef());
   Value *NSV1B = Builder.CreateShuffleVector(GetShuffleOperand(SVI1B, 0),
                                              GetShuffleOperand(SVI1B, 1), V2B);
   Builder.SetInsertPoint(Op0);



More information about the llvm-commits mailing list