[DebugInfo][RemoveDIs] Use iterators to insert everywhere

[jmorse]

These are the final few places in LLVM where we use instruction pointers to identify the position that we're inserting something. We're trying to get away from that with a view to deprecating those methods, thus use iterators in all these places. I believe they're all debug-info safe.

The sketchiest part is the ExtractValueInst copy constructor, where we cast nullptr to a BasicBlock pointer, so that we take the non-default insert-into-no-block path for instruction insertion, instead of the default nullptr-instruction path for UnaryInstruction. Such a hack is necessary until we get rid of the instruction constructor entirely.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-llvm-transforms

Author: Jeremy Morse (jmorse)

Changes

These are the final few places in LLVM where we use instruction pointers to identify the position that we're inserting something. We're trying to get away from that with a view to deprecating those methods, thus use iterators in all these places. I believe they're all debug-info safe.

The sketchiest part is the ExtractValueInst copy constructor, where we cast nullptr to a BasicBlock pointer, so that we take the non-default insert-into-no-block path for instruction insertion, instead of the default nullptr-instruction path for UnaryInstruction. Such a hack is necessary until we get rid of the instruction constructor entirely.

---

Full diff: https://github.com/llvm/llvm-project/pull/102003.diff

6 Files Affected:

• (modified) llvm/lib/IR/Instructions.cpp (+1-1)
• (modified) llvm/lib/Transforms/IPO/ExpandVariadics.cpp (+1-1)
• (modified) llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp (+2-1)
• (modified) llvm/lib/Transforms/Scalar/LICM.cpp (+5-3)
• (modified) llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp (+1-1)
• (modified) llvm/lib/Transforms/Vectorize/LoopVectorize.cpp (+1-1)

diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp
index 58ebe7e95cd06..4bf3ca8968ba5 100644
--- a/llvm/lib/IR/Instructions.cpp
+++ b/llvm/lib/IR/Instructions.cpp
@@ -2484,7 +2484,7 @@ void ExtractValueInst::init(ArrayRef<unsigned> Idxs, const Twine &Name) {
 }
 
 ExtractValueInst::ExtractValueInst(const ExtractValueInst &EVI)
-  : UnaryInstruction(EVI.getType(), ExtractValue, EVI.getOperand(0)),
+  : UnaryInstruction(EVI.getType(), ExtractValue, EVI.getOperand(0), (BasicBlock*)nullptr),
     Indices(EVI.Indices) {
   SubclassOptionalData = EVI.SubclassOptionalData;
 }
diff --git a/llvm/lib/Transforms/IPO/ExpandVariadics.cpp b/llvm/lib/Transforms/IPO/ExpandVariadics.cpp
index b5b590e2b7acf..3a1f690bf0390 100644
--- a/llvm/lib/Transforms/IPO/ExpandVariadics.cpp
+++ b/llvm/lib/Transforms/IPO/ExpandVariadics.cpp
@@ -809,7 +809,7 @@ bool ExpandVariadics::expandCall(Module &M, IRBuilder<> &Builder, CallBase *CB,
     Value *Dst = NF ? NF : CI->getCalledOperand();
     FunctionType *NFTy = inlinableVariadicFunctionType(M, VarargFunctionType);
 
-    NewCB = CallInst::Create(NFTy, Dst, Args, OpBundles, "", CI);
+    NewCB = CallInst::Create(NFTy, Dst, Args, OpBundles, "", CI->getIterator());
 
     CallInst::TailCallKind TCK = CI->getTailCallKind();
     assert(TCK != CallInst::TCK_MustTail);
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 9fb1df7ab2b79..147925eedb440 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -691,7 +691,8 @@ class RuntimeCallInserter {
         // Replace CI with a clone with an added funclet OperandBundle
         OperandBundleDef OB("funclet", EHPad);
         auto *NewCall =
-            CallBase::addOperandBundle(CI, LLVMContext::OB_funclet, OB, CI);
+            CallBase::addOperandBundle(CI, LLVMContext::OB_funclet, OB,
+                                       CI->getIterator());
         NewCall->copyMetadata(*CI);
         CI->replaceAllUsesWith(NewCall);
         CI->eraseFromParent();
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 329b3ef0c8e4b..0a60bf1eb4fbf 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -2768,7 +2768,8 @@ static bool hoistMulAddAssociation(Instruction &I, Loop &L,
     auto *LHS = OpIdx == 0 ? Mul : Ins->getOperand(0);
     auto *RHS = OpIdx == 1 ? Mul : Ins->getOperand(1);
     auto *NewBO = BinaryOperator::Create(Ins->getOpcode(), LHS, RHS,
-                                         Ins->getName() + ".reass", Ins);
+                                         Ins->getName() + ".reass",
+                                         Ins->getIterator());
     NewBO->copyIRFlags(Ins);
     if (VariantOp == Ins)
       VariantOp = NewBO;
@@ -2821,9 +2822,10 @@ static bool hoistBOAssociation(Instruction &I, Loop &L,
   assert(Preheader && "Loop is not in simplify form?");
 
   auto *Inv = BinaryOperator::Create(Opcode, C1, C2, "invariant.op",
-                                     Preheader->getTerminator());
+                                     Preheader->getTerminator()->getIterator());
   auto *NewBO =
-      BinaryOperator::Create(Opcode, LV, Inv, BO->getName() + ".reass", BO);
+      BinaryOperator::Create(Opcode, LV, Inv, BO->getName() + ".reass",
+                             BO->getIterator());
 
   // Copy NUW for ADDs if both instructions have it.
   if (Opcode == Instruction::Add && BO->hasNoUnsignedWrap() &&
diff --git a/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp b/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
index 546a6cd56b250..8410daeed7ad0 100644
--- a/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
+++ b/llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
@@ -462,7 +462,7 @@ static void convertMetadataToAssumes(LoadInst *LI, Value *Val,
     LLVMContext &Ctx = LI->getContext();
     new StoreInst(ConstantInt::getTrue(Ctx),
                   PoisonValue::get(PointerType::getUnqual(Ctx)),
-                  /*isVolatile=*/false, Align(1), LI);
+                  /*isVolatile=*/false, Align(1), LI->getIterator());
     return;
   }
 
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 6daa8043a3fbf..95d1b463edb30 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -2633,7 +2633,7 @@ PHINode *InnerLoopVectorizer::createInductionResumeValue(
 
   // Create phi nodes to merge from the  backedge-taken check block.
   PHINode *BCResumeVal = PHINode::Create(OrigPhi->getType(), 3, "bc.resume.val",
-                                         LoopScalarPreHeader->getFirstNonPHI());
+                                         LoopScalarPreHeader->getFirstNonPHIIt());
   // Copy original phi DL over to the new one.
   BCResumeVal->setDebugLoc(OrigPhi->getDebugLoc());
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[OCHyams]

LGTM

[jmorse]

This is unexpectedly slowing -O3 builds a little bit: https://llvm-compile-time-tracker.com/compare.php?from=1139dee910c45cfdd4a6066b22addef585e04032&to=fd7d7882e7fa5a38d4bfde426120d4663718beb4&stat=instructions%3Au

Which is especially unexpected seeing how it's a tiny number of call sites that shouldn't reeeaaalllyyy be behaving any differently. One wonders if it's some second-order inlining thing. I'll examine a little shortly.

[jmorse]

IT's this hunk: 1e05c3a

That produces most of the slowdown, 0.05% total. Not sure there's really anything to do about it.

I believe I originally promised this would all pay for itself when we removed all the branching / skipping of debug intrinsics in various passes, but we're not at the point of actually stripping those out yet.