Improve the API of DILocation::getMergedLocation()

[aprantl]

Summary

In a recent thread on LLVM-dev "Stripping Debug Locations on cross BB moves, part 2 (PR31891)" we discovered that the existing API for
getMergedLocation() is unsafe when used on CallInstructions. This patch moves the API from DILocation on Instruction so it automatically behaves correctly (i.e. return a line-0 debug loc) when used on a call instruction.

Once we agree on the exact API, I will split the patch up into an NFC commit that just moves the API and is a NOP on CallInstructions, and a second one that returns the line-0 location for CallInstructions.

[dblaikie]

Also we should have a function for the non-merging case (hoisting/sinking) so we can do similar things there (& presumably refactor the common infrastructure between these two operations).

lib/IR/Instruction.cpp

664–668

I'm not sure we can get the scope right, though - since we're potentially moving this location across scopes. It's going to be jumpy/create a difficult range in the scope no matter what, really...

Any ideas? Take the scope from a nearby location at the destination, if possible? (doesn't really reflect reality, but nothing will - this would at least mean a chance of not punching holes in/making islands in scopes that would necessitate a DW_AT_ranges/more verbose description/etc)

[twoh]

lib/IR/Instruction.cpp

664–668

It would be hard to get the precise scope anyway, but I think it might be worth to consider the inlining context. Please take a look at the following test case:

define i32 @bar() !dbg !6 {
entry:
  %call = tail call i32 @foo(), !dbg !8
  ret i32 %call, !dbg !9
}

define i32 @test(i32 %a, i32 %b) !dbg !10 {
entry:
  %tobool = icmp ne i32 %a, 0, !dbg !11
  br i1 %tobool, label %if.then, label %if.else, !dbg !11

if.then:                                          ; preds = %entry
  %call.i = call i32 @foo(), !dbg !12
  %sub = sub nsw i32 %b, %call.i, !dbg !14
  br label %if.end, !dbg !15

if.else:                                          ; preds = %entry
  %call1 = call i32 @foo(), !dbg !16
  %sub2 = sub nsw i32 %b, %call1, !dbg !17
  br label %if.end

if.end:                                           ; preds = %if.else, %if.then
  %b.addr.0 = phi i32 [ %sub, %if.then ], [ %sub2, %if.else ]
  ret i32 %b.addr.0, !dbg !18
}

where

!6 = distinct !DISubprogram(name: "bar", ...)
!10 = distinct !DISubprogram(name: "test", ...)
!12 = !DILocation(line: 4, column: 10, scope: !6, inlinedAt: !13)
!16 = !DILocation(line: 11, column: 10, scope: !10)

Here the call instruction in %if.then block is actually inlined from @bar, and if we pickup the scope from that instruction, the final code would be look something like below:

define i32 @test(i32 %a, i32 %b) !dbg !10 {
entry:
  %call.i = call i32 @foo(), !dbg !11
  %sub = sub nsw i32 %b, %call.i, !dbg !13
  ret i32 %sub, !dbg !14
}

!6 = distinct !DISubprogram(name: "bar", ...)
!11 = !DILocation(line: 0, column: 0, scope: !6, inlinedAt: !12)

As the call instructions are merged and hoisted inside test not bar, it would be awkward if the merged call instruction has a scope of bar, though the debug location is still just (line: 0, column: 0).

I missed the discussion in llvm-dev and implemented a patch for the same issue considering inline scope (D29927). Maybe worth take a look.

[aprantl]

Ping! Sorry, I've been distracted. How do we want to proceed here?

I think the first action item we should get out of the way is:

• Do we think that moving mergeDebugLocWith into Instruction is a a good idea?

Once we decide on that issue, let's talk about how to best merge D29833 and D29927.

[dblaikie]

I don't have strong feelings about where this function lives. Still unsure about preserving the scope from either of the merged locations, though.

[twoh]

Personally I prefer the existing API, because it doesn't allow the transient state before merging. For example, in lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch, DebugLoc of NewSI is not actually valid after line 1430. Existing API doesn't have this problem.

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

[probinson]

In D29833#682595, @twoh wrote:

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

Mmmm... Does scope imply source file? LTO can inline across CUs, then merging an instruction from CU#1 into a scope from CU#2 would make the line number meaningless.

[twoh]

In D29833#682865, @probinson wrote:

In D29833#682595, @twoh wrote:

I don't have strong opinion about scope, because anyway we won't be able to have precise scope. Taking DISubprogram of the destination instruction might be a conservative option.

Mmmm... Does scope imply source file? LTO can inline across CUs, then merging an instruction from CU#1 into a scope from CU#2 would make the line number meaningless.

My understanding is that if two instructions come from different source file, canDiscriminate will return true because filenames are different and the merged metadata will have a line 0 location.

[aprantl]

[apologies for iterating so slowly at the moment]

In D29833#682595, @twoh wrote:

Personally I prefer the existing API, because it doesn't allow the transient state before merging. For example, in lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp in this patch, DebugLoc of NewSI is not actually valid after line 1430. Existing API doesn't have this problem.

If that is the only concern, would an additional convenience function solve (= sufficiently hide) the problem?

Instruction::setMergedDebugLoc(DebugLoc A, DebugLoc B) {
  setDebugLoc(A);
  mergeDebugLocWith(B);
}

The primary benefit of having the API on Instruction is that we can make it impossible to accidentally create a locationless call instruction in a function with debuginfo (which would crash/assert the backend when creating inline scopes).

[twoh]

@aprantl I like the idea of having a convenience function. I'm afraid I still don't understand @dblaikie on getMergedLocation is better than the proposed convenience function in preventing buggy profile.

[aprantl]

I'm abandoning this revision in favor of extending getMergedLocation as discussed in D29927.

include/llvm/IR/DebugInfoMetadata.h

public:
   ///
   /// DWARF discriminators distinguish identical file locations between
   /// instructions that are on different basic blocks.
   inline unsigned getDiscriminator() const;
 
   /// Returns a new DILocation with updated \p Discriminator.
   inline DILocation *cloneWithDiscriminator(unsigned Discriminator) const;
 
-  /// When two instructions are combined into a single instruction we also
-  /// need to combine the original locations into a single location.
-  ///
-  /// When the locations are the same we can use either location. When they
-  /// differ, we need a third location which is distinct from either. If
-  /// they have the same file/line but have a different discriminator we
-  /// could create a location with a new discriminator. If they are from
-  /// different files/lines the location is ambiguous and can't be
-  /// represented in a single line entry.  In this case, no location
-  /// should be set.
-  ///
-  /// Currently the function does not create a new location. If the locations
-  /// are the same, or cannot be discriminated, the first location is returned.
-  /// Otherwise an empty location will be used.
-  static const DILocation *getMergedLocation(const DILocation *LocA,
-                                             const DILocation *LocB) {
-    if (LocA && LocB && (LocA == LocB || !LocA->canDiscriminate(*LocB)))
-      return LocA;
-    return nullptr;
-  }
-
   Metadata *getRawScope() const { return getOperand(0); }
   Metadata *getRawInlinedAt() const {
     if (getNumOperands() == 2)
       return getOperand(1);
     return nullptr;
   }
 
   static bool classof(const Metadata *MD) {

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; }
 
+  /// When two instructions are combined into a single instruction we also
+  /// need to combine the original locations into a single location.
+  ///
+  /// When the locations are the same we can use either location. When they
+  /// differ, we need a third location which is distinct from either. If
+  /// they have the same file/line but have a different discriminator we
+  /// could create a location with a new discriminator. If they are from
+  /// different files/lines the location is ambiguous and can't be
+  /// represented in a single line entry.  In this case, no location
+  /// should be set.
+  ///
+  /// Currently the function does not create a new location. If the locations
+  /// are the same, or cannot be discriminated, the first location is returned.
+  /// Otherwise an empty location will be used.
+  void mergeDebugLocWith(const DILocation *Other);
+
   /// 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 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);
 

lib/IR/Instruction.cpp

 // This file implements the Instruction class for the IR library.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/Type.h"
 using namespace llvm;
 
 Instruction::Instruction(Type *ty, unsigned it, Use *Ops, unsigned NumOps,
                          Instruction *InsertBefore)

 #include "llvm/IR/Instruction.def"
 #undef HANDLE_INST
   }
 
   New->SubclassOptionalData = SubclassOptionalData;
   New->copyMetadata(*this);
   return New;
 }
+
+void Instruction::mergeDebugLocWith(const DILocation *Other) {
+  // If they are equivalent, keep the current location.
+  DILocation *Loc = DbgLoc.get();
+  if (Loc && Other && (Loc == Other || !Loc->canDiscriminate(*Other)))
+    return;
+
+  // If this is a call instruction, replace it with a line 0 location in the
+  // correct scope.
+  if (Loc && isa<CallInst>(this)) {
+    DbgLoc = DILocation::get(Loc->getContext(), 0, 0, Loc->getScope(),
+                             Loc->getInlinedAt());
+    return;
+  }
+
+  // Else delete it.
+  DbgLoc = {};
+}

lib/Transforms/InstCombine/InstCombineInternal.h

private:
   /// \brief Try to rotate an operation below a PHI node, using PHI nodes for
   /// its operands.
   Instruction *FoldPHIArgOpIntoPHI(PHINode &PN);
   Instruction *FoldPHIArgBinOpIntoPHI(PHINode &PN);
   Instruction *FoldPHIArgGEPIntoPHI(PHINode &PN);
   Instruction *FoldPHIArgLoadIntoPHI(PHINode &PN);
   Instruction *FoldPHIArgZextsIntoPHI(PHINode &PN);
 
-  /// Helper function for FoldPHIArgXIntoPHI() to get debug location for the
-  /// folded operation.
-  DebugLoc PHIArgMergedDebugLoc(PHINode &PN);
-
   Instruction *foldGEPICmp(GEPOperator *GEPLHS, Value *RHS,
                            ICmpInst::Predicate Cond, Instruction &I);
   Instruction *foldAllocaCmp(ICmpInst &ICI, const AllocaInst *Alloca,
                              const Value *Other);
   Instruction *foldCmpLoadFromIndexedGlobal(GetElementPtrInst *GEP,
                                             GlobalVariable *GV, CmpInst &ICI,
                                             ConstantInt *AndCst = nullptr);
   Instruction *foldFCmpIntToFPConst(FCmpInst &I, Instruction *LHSI,

lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

bool InstCombiner::SimplifyStoreAtEndOfBlock(StoreInst &SI) {
   BBI = DestBB->getFirstInsertionPt();
   StoreInst *NewSI = new StoreInst(MergedVal, SI.getOperand(1),
                                    SI.isVolatile(),
                                    SI.getAlignment(),
                                    SI.getOrdering(),
                                    SI.getSynchScope());
   InsertNewInstBefore(NewSI, *BBI);
   // The debug locations of the original instructions might differ; merge them.
-  NewSI->setDebugLoc(DILocation::getMergedLocation(SI.getDebugLoc(),
-                                                   OtherStore->getDebugLoc()));
+  NewSI->setDebugLoc(SI.getDebugLoc());
+  NewSI->mergeDebugLocWith(OtherStore->getDebugLoc());
 
   // If the two stores had AA tags, merge them.
   AAMDNodes AATags;
   SI.getAAMetadata(AATags);
   if (AATags) {
     OtherStore->getAAMetadata(AATags, /* Merge = */ true);
     NewSI->setAAMetadata(AATags);
   }
 
   // Nuke the old stores.
   eraseInstFromFunction(SI);
   eraseInstFromFunction(*OtherStore);
   return true;
 }

lib/Transforms/InstCombine/InstCombinePHI.cpp

 using namespace llvm;
 using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "instcombine"
 
 /// The PHI arguments will be folded into a single operation with a PHI node
 /// as input. The debug location of the single operation will be the merged
 /// locations of the original PHI node arguments.
-DebugLoc InstCombiner::PHIArgMergedDebugLoc(PHINode &PN) {
-  auto *FirstInst = cast<Instruction>(PN.getIncomingValue(0));
-  const DILocation *Loc = FirstInst->getDebugLoc();
-
-  for (unsigned i = 1; i != PN.getNumIncomingValues(); ++i) {
+static void setPHIArgMergedDebugLoc(Instruction *Inst, PHINode &PN) {
+  for (unsigned i = 0; i != PN.getNumIncomingValues(); ++i) {
     auto *I = cast<Instruction>(PN.getIncomingValue(i));
-    Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc());
+    Inst->mergeDebugLocWith(I->getDebugLoc());
   }
-
-  return Loc;
 }
 
 /// If we have something like phi [add (a,b), add(a,c)] and if a/b/c and the
 /// adds all have a single use, turn this into a phi and a single binop.
 Instruction *InstCombiner::FoldPHIArgBinOpIntoPHI(PHINode &PN) {
   Instruction *FirstInst = cast<Instruction>(PN.getIncomingValue(0));
   assert(isa<BinaryOperator>(FirstInst) || isa<CmpInst>(FirstInst));
   unsigned Opc = FirstInst->getOpcode();
for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) {
         NewRHS->addIncoming(NewInRHS, PN.getIncomingBlock(i));
       }
     }
   }
 
   if (CmpInst *CIOp = dyn_cast<CmpInst>(FirstInst)) {
     CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(),
                                      LHSVal, RHSVal);
-    NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
+    setPHIArgMergedDebugLoc(NewCI, PN);
     return NewCI;
   }
 
   BinaryOperator *BinOp = cast<BinaryOperator>(FirstInst);
   BinaryOperator *NewBinOp =
     BinaryOperator::Create(BinOp->getOpcode(), LHSVal, RHSVal);
 
   NewBinOp->copyIRFlags(PN.getIncomingValue(0));
 
   for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i)
     NewBinOp->andIRFlags(PN.getIncomingValue(i));
 
-  NewBinOp->setDebugLoc(PHIArgMergedDebugLoc(PN));
+  setPHIArgMergedDebugLoc(NewBinOp, PN);
   return NewBinOp;
 }
 
 Instruction *InstCombiner::FoldPHIArgGEPIntoPHI(PHINode &PN) {
   GetElementPtrInst *FirstInst =cast<GetElementPtrInst>(PN.getIncomingValue(0));
 
   SmallVector<Value*, 16> FixedOperands(FirstInst->op_begin(),
                                         FirstInst->op_end());
for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) {
           OpPhi->addIncoming(InGEP->getOperand(op), InBB);
     }
   }
 
   Value *Base = FixedOperands[0];
   GetElementPtrInst *NewGEP =
       GetElementPtrInst::Create(FirstInst->getSourceElementType(), Base,
                                 makeArrayRef(FixedOperands).slice(1));
-  if (AllInBounds) NewGEP->setIsInBounds();
-  NewGEP->setDebugLoc(PHIArgMergedDebugLoc(PN));
+  if (AllInBounds)
+    NewGEP->setIsInBounds();
+  setPHIArgMergedDebugLoc(NewGEP, PN);
   return NewGEP;
 }
 
 
 /// Return true if we know that it is safe to sink the load out of the block
 /// that defines it. This means that it must be obvious the value of the load is
 /// not changed from the point of the load to the end of the block it is in.
 ///
Instruction *InstCombiner::FoldPHIArgLoadIntoPHI(PHINode &PN) {
 
   // If this was a volatile load that we are merging, make sure to loop through
   // and mark all the input loads as non-volatile.  If we don't do this, we will
   // insert a new volatile load and the old ones will not be deletable.
   if (isVolatile)
     for (Value *IncValue : PN.incoming_values())
       cast<LoadInst>(IncValue)->setVolatile(false);
 
-  NewLI->setDebugLoc(PHIArgMergedDebugLoc(PN));
+  setPHIArgMergedDebugLoc(NewLI, PN);
   return NewLI;
 }
 
 /// TODO: This function could handle other cast types, but then it might
 /// require special-casing a cast from the 'i1' type. See the comment in
 /// FoldPHIArgOpIntoPHI() about pessimizing illegal integer types.
 Instruction *InstCombiner::FoldPHIArgZextsIntoPHI(PHINode &Phi) {
   // We cannot create a new instruction after the PHI if the terminator is an
if (InVal) {
     InsertNewInstBefore(NewPN, PN);
     PhiVal = NewPN;
   }
 
   // Insert and return the new operation.
   if (CastInst *FirstCI = dyn_cast<CastInst>(FirstInst)) {
     CastInst *NewCI = CastInst::Create(FirstCI->getOpcode(), PhiVal,
                                        PN.getType());
-    NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
+    setPHIArgMergedDebugLoc(NewCI, PN);
     return NewCI;
   }
 
   if (BinaryOperator *BinOp = dyn_cast<BinaryOperator>(FirstInst)) {
     BinOp = BinaryOperator::Create(BinOp->getOpcode(), PhiVal, ConstantOp);
     BinOp->copyIRFlags(PN.getIncomingValue(0));
 
     for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i)
       BinOp->andIRFlags(PN.getIncomingValue(i));
 
-    BinOp->setDebugLoc(PHIArgMergedDebugLoc(PN));
+    setPHIArgMergedDebugLoc(BinOp, PN);
     return BinOp;
   }
 
   CmpInst *CIOp = cast<CmpInst>(FirstInst);
   CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(),
                                    PhiVal, ConstantOp);
-  NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
+  setPHIArgMergedDebugLoc(NewCI, PN);
   return NewCI;
 }
 
 /// Return true if this PHI node is only used by a PHI node cycle that is dead.
 static bool DeadPHICycle(PHINode *PN,
                          SmallPtrSetImpl<PHINode*> &PotentiallyDeadPHIs) {
   if (PN->use_empty()) return true;
   if (!PN->hasOneUse()) return false;

lib/Transforms/Utils/SimplifyCFG.cpp

unsigned KnownIDs[] = {LLVMContext::MD_tbaa,
                            LLVMContext::MD_align,
                            LLVMContext::MD_dereferenceable,
                            LLVMContext::MD_dereferenceable_or_null,
                            LLVMContext::MD_mem_parallel_loop_access};
     combineMetadata(I1, I2, KnownIDs);
 
     // I1 and I2 are being combined into a single instruction.  Its debug
     // location is the merged locations of the original instructions.
-    if (!isa<CallInst>(I1))
-      I1->setDebugLoc(
-          DILocation::getMergedLocation(I1->getDebugLoc(), I2->getDebugLoc()));
+    I1->mergeDebugLocWith(I2->getDebugLoc());
 
     I2->eraseFromParent();
     Changed = true;
 
     I1 = &*BB1_Itr++;
     I2 = &*BB2_Itr++;
     // Skip debug info if it is not identical.
     DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
   // and move it to the start of the successor block.
   for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O)
     I0->getOperandUse(O).set(NewOperands[O]);
   I0->moveBefore(&*BBEnd->getFirstInsertionPt());
 
   // The debug location for the "common" instruction is the merged locations of
   // all the commoned instructions.  We start with the original location of the
   // "common" instruction and iteratively merge each location in the loop below.
-  const DILocation *Loc = I0->getDebugLoc();
 
   // Update metadata and IR flags, and merge debug locations.
   for (auto *I : Insts)
     if (I != I0) {
-      Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc());
+      I0->mergeDebugLocWith(I->getDebugLoc());
       combineMetadataForCSE(I0, I);
       I0->andIRFlags(I);
     }
-  if (!isa<CallInst>(I0))
-    I0->setDebugLoc(Loc);
 
   if (!isa<StoreInst>(I0)) {
     // canSinkLastInstruction checked that all instructions were used by
     // one and only one PHI node. Find that now, RAUW it to our common
     // instruction and nuke it.
     assert(I0->hasOneUse());
     auto *PN = cast<PHINode>(*I0->user_begin());
     PN->replaceAllUsesWith(I0);
if (SpeculatedStoreValue) {
     IRBuilder<NoFolder> Builder(BI);
     Value *TrueV = SpeculatedStore->getValueOperand();
     Value *FalseV = SpeculatedStoreValue;
     if (Invert)
       std::swap(TrueV, FalseV);
     Value *S = Builder.CreateSelect(
         BrCond, TrueV, FalseV, TrueV->getName() + "." + FalseV->getName(), BI);
     SpeculatedStore->setOperand(0, S);
-    SpeculatedStore->setDebugLoc(
-        DILocation::getMergedLocation(
-          BI->getDebugLoc(), SpeculatedStore->getDebugLoc()));
+    SpeculatedStore->mergeDebugLocWith(BI->getDebugLoc());
   }
 
   // Metadata can be dependent on the condition we are hoisting above.
   // Conservatively strip all metadata on the instruction.
   for (auto &I : *ThenBB)
     I.dropUnknownNonDebugMetadata();
 
   // Hoist the instructions.