[Instruction] Add updateLocationAfterHoist helper

[vsk]

Summary

Introduce a helper which can be used to update the debug location of an
Instruction after the instruction is hoisted. This can be used to safely
drop a source location as recommended by the docs.

For more context, see the discussion in https://reviews.llvm.org/D60913.

[vsk]

Add doxygen comment.

[aprantl]

Thank you!

[vsk]

llvm/lib/IR/DebugInfo.cpp

714

This causes some bots to fail with "attachment points at wrong subprogram for function" verifier errors. The verifier check tests that a location's inlinedAt scope describes the parent function it's in. I believe the correct way to handle calls which have a location is to set the parent function's scope on the call (if the parent function has a scope), and to fall back to using DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()) if not.

[vsk]

Preserve the inlinedAt location unless the parent function has a scope.

[aprantl]

llvm/lib/IR/DebugInfo.cpp

717

Should we pick up a scope from the instruction before it to avoid having a hole in that scope's range?

[vsk]

Add a dropLocation API, and use this to implement updateLocationAfterHoist. 'dropLocation' is a more general name, and should be applicable in more contexts.

llvm/lib/IR/DebugInfo.cpp

717

I'd argue that we shouldn't. In this case, I think we want to use a scope that conveys 'you could be anywhere in this function'. The scope from a nearby instruction could be more specific than that.

[dblaikie]

llvm/lib/IR/DebugInfo.cpp

717

Yeah, this one's unfortunate - when merging, we can find the nearest common scope and that's at least somewhat meaningful. But when hoisting that's not so clearly correct. :/

723–724

This alternative seems a bit problematic, as it may lead to differences in location depending on the order of inlining, which might be problematic/volatile/unpredictable for developers? but maybe there's no good answer. The other option only seems to be "drop the location entirely" (I think that's correct in the case where it isn't in a function with debug info? IF it gets inlined into such a function, it'll get a location at that point?)

[vsk]

• To drop the location of a call within a function with no subprogram, set an empty DebugLoc instead of trying to preserve scope/inlinedAt info. Update a unit test accordingly.

[vsk]

llvm/lib/IR/DebugInfo.cpp

723–724

Thanks for the suggestion. I wasn't aware that the inliner provided a location when inlining a call without a location from a function without a subprogram. But I suppose it must, because the inlined call may reference a function that /does/ have a subprogram attached.

Dropping the location seems less arbitrary than preserving the old scope/inlinedAt info.

[dblaikie]

Seems good to me - minor tidy up might be possible in the unit test.

llvm/unittests/IR/InstructionsTest.cpp

1350

Maybe these should be EXPECT_EQ(I1->getDebugLoc(), DebugLoc());? Would that work? Would hopefully give better failure messages by printing the values when the expect fails.

[vsk]

llvm/unittests/IR/InstructionsTest.cpp

1350

That does seem cleaner, thanks. I'll fix this up before committing.

llvm/include/llvm/IR/Instruction.h

public:
   ///     O(2*N*I), where N is # of Instructions to merge, and I is the
   ///     maximum level of inline stack. So it is still linear.
   ///  3) Merging of call instructions should be extremely rare in real
   ///     applications, thus the N-way merging should be in code path.
   /// The DebugLoc attached to this instruction will be overwritten by the
   /// merged DebugLoc.
   void applyMergedLocation(const DILocation *LocA, const DILocation *LocB);
 
+  /// Updates the debug location given that the instruction has been hoisted
+  /// from a block to a predecessor of that block.
+  void updateLocationAfterHoist();
+
 private:
   /// Return true if we have an entry in the on-the-side metadata hash.
   bool hasMetadataHashEntry() const {
     return Bitfield::test<HasMetadataField>(getSubclassDataFromValue());
   }
 
   // These are all implemented in Metadata.cpp.
   MDNode *getMetadataImpl(unsigned KindID) const;

llvm/lib/IR/DebugInfo.cpp

unsigned llvm::getDebugMetadataVersionFromModule(const Module &M) {
   return 0;
 }
 
 void Instruction::applyMergedLocation(const DILocation *LocA,
                                       const DILocation *LocB) {
   setDebugLoc(DILocation::getMergedLocation(LocA, LocB));
 }
 
+void Instruction::updateLocationAfterHoist() {
+  const DebugLoc &DL = getDebugLoc();
+  if (!DL)
+    return;
+
+  // If we didn't hoist a call, drop the location to allow a location from a
+  // preceding instruction to propagate.
+  if (!isa<CallBase>(this)) {
+    setDebugLoc(DebugLoc());
+    return;
+  }
+
+  // Set a line 0 location for (potentially inlinable) calls. Set the scope to
+  // the parent function's scope if it's available, and drop any inlinedAt info
+  // to avoid making it look like the inlined callee was reached early.
+  setDebugLoc(DebugLoc::get(0, 0, DL.getScope()));
+}
+
 //===----------------------------------------------------------------------===//
 // LLVM C API implementations.
 //===----------------------------------------------------------------------===//
 
 static unsigned map_from_llvmDWARFsourcelanguage(LLVMDWARFSourceLanguage lang) {
   switch (lang) {
 #define HANDLE_DW_LANG(ID, NAME, LOWER_BOUND, VERSION, VENDOR)                 \
   case LLVMDWARFSourceLanguage##NAME:                                          \
     return ID;
 #include "llvm/BinaryFormat/Dwarf.def"
 #undef HANDLE_DW_LANG
   }
   llvm_unreachable("Unhandled Tag");
 }
 
 template <typename DIT> DIT *unwrapDI(LLVMMetadataRef Ref) {

llvm/lib/Transforms/Scalar/GVN.cpp

 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constant.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
-#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
LLVM_DEBUG(if (!NewInsts.empty()) dbgs()
              << "INSERTED " << NewInsts.size() << " INSTS: " << *NewInsts.back()
              << '\n');
 
   // Assign value numbers to the new instructions.
   for (Instruction *I : NewInsts) {
     // Instructions that have been inserted in predecessor(s) to materialize
     // the load address do not retain their original debug locations. Doing
     // so could lead to confusing (but correct) source attributions.
-    if (const DebugLoc &DL = I->getDebugLoc())
-      I->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
+    I->updateLocationAfterHoist();
 
     // FIXME: We really _ought_ to insert these value numbers into their
     // parent's availability map.  However, in doing so, we risk getting into
     // ordering issues.  If a block hasn't been processed yet, we would be
     // marking a value as AVAIL-IN, which isn't what we intend.
     VN.lookupOrAdd(I);
   }
 

llvm/lib/Transforms/Scalar/LICM.cpp

static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
 
   if (isa<PHINode>(I))
     // Move the new node to the end of the phi list in the destination block.
     moveInstructionBefore(I, *Dest->getFirstNonPHI(), *SafetyInfo, MSSAU, SE);
   else
     // Move the new node to the destination block, before its terminator.
     moveInstructionBefore(I, *Dest->getTerminator(), *SafetyInfo, MSSAU, SE);
 
-  // Apply line 0 debug locations when we are moving instructions to different
-  // basic blocks because we want to avoid jumpy line tables.
-  if (const DebugLoc &DL = I.getDebugLoc())
-    I.setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
+  I.updateLocationAfterHoist();
 
   if (isa<LoadInst>(I))
     ++NumMovedLoads;
   else if (isa<CallInst>(I))
     ++NumMovedCalls;
   ++NumHoisted;
 }
 

llvm/test/DebugInfo/Generic/licm-hoist-debug-loc.ll

 ;    for (int i = 0; i < k; i++) {
 ;      bar(&p + 4);
 ;    }
 ; }
 ;
 ; We make sure that the instruction that is hoisted into the preheader
 ; does not have a debug location.
 ; CHECK: for.body.lr.ph:
-; CHECK: getelementptr{{.*}}%p.addr, i64 4{{.*}} !dbg [[zero:![0-9]+]]
+; CHECK: getelementptr{{.*}}%p.addr, i64 4{{$}}
 ; CHECK: for.body:
-; CHECK: [[zero]] = !DILocation(line: 0
 ;
 ; ModuleID = 't.ll'
 source_filename = "test.c"
 
 ; Function Attrs: nounwind sspstrong uwtable
 define void @foo(i32 %k, i32 %p) !dbg !7 {
 entry:
   %p.addr = alloca i32, align 4

llvm/test/Transforms/GVN/PRE/phi-translate.ll

 ; RUN: opt -basic-aa -gvn -S < %s | FileCheck %s
 
 target datalayout = "e-p:64:64:64"
 
 ; CHECK-LABEL: @foo(
 ; CHECK: entry.end_crit_edge:
-; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{.*}} !dbg [[ZERO_LOC:![0-9]+]]
-; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{.*}} !dbg [[ZERO_LOC]]
+; CHECK: %[[INDEX:[a-z0-9.]+]] = sext i32 %x to i64{{$}}
+; CHECK: %[[ADDRESS:[a-z0-9.]+]] = getelementptr [100 x i32], [100 x i32]* @G, i64 0, i64 %[[INDEX]]{{$}}
 ; CHECK:   %n.pre = load i32, i32* %[[ADDRESS]], align 4, !dbg [[N_LOC:![0-9]+]]
 ; CHECK: br label %end
 ; CHECK: then:
 ; CHECK:   store i32 %z
 ; CHECK: end:
 ; CHECK:   %n = phi i32 [ %n.pre, %entry.end_crit_edge ], [ %z, %then ], !dbg [[N_LOC]]
 ; CHECK:   ret i32 %n
 
-; CHECK-DAG: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
-; CHECK-DAG: [[ZERO_LOC]] = !DILocation(line: 0
+; CHECK: [[N_LOC]] = !DILocation(line: 47, column: 1, scope: !{{.*}})
 
 @G = external global [100 x i32]
 define i32 @foo(i32 %x, i32 %z) !dbg !6 {
 entry:
   %tobool = icmp eq i32 %z, 0, !dbg !7
   br i1 %tobool, label %end, label %then, !dbg !7
 
 then:

llvm/test/Transforms/LICM/hoisting-preheader-debugloc.ll

 ; RUN: opt -S -licm < %s | FileCheck %s
 
-; CHECK: %arrayidx4.promoted = load i32, i32* %arrayidx4, align 1, !tbaa !59
+; CHECK: %arrayidx4.promoted = load i32, i32* %arrayidx4, align 1, !tbaa !{{[0-9]+$}}
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 @a = dso_local local_unnamed_addr global i16 0, align 2, !dbg !0
 @b = dso_local local_unnamed_addr global i16 0, align 2, !dbg !6
 
 define dso_local void @c(i32 %d) local_unnamed_addr #0 !dbg !13 {

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, UnaryOperator) {
   EXPECT_TRUE(isa<UnaryInstruction>(F));
   EXPECT_TRUE(isa<UnaryOperator>(F));
   EXPECT_FALSE(isa<BinaryOperator>(F));
 
   F->deleteValue();
   I->deleteValue();
 }
 
+TEST(InstructionsTest, UpdateLocationAfterHoist) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = parseIR(C,
+                                      R"(
+      declare void @callee()
+
+      define void @f() {
+        call void @callee()           ; Inst with no location.
+        call void @callee(), !dbg !11 ; Call with location.
+        ret void, !dbg !11            ; Non-call inst with location.
+      }
+
+      !llvm.dbg.cu = !{!0}
+      !llvm.module.flags = !{!3, !4}
+      !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 6.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+      !1 = !DIFile(filename: "t2.c", directory: "foo")
+      !2 = !{}
+      !3 = !{i32 2, !"Dwarf Version", i32 4}
+      !4 = !{i32 2, !"Debug Info Version", i32 3}
+      !8 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !9, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: false, unit: !0, retainedNodes: !2)
+      !9 = !DISubroutineType(types: !10)
+      !10 = !{null}
+      !11 = !DILocation(line: 2, column: 7, scope: !8)
+  )");
+  ASSERT_TRUE(M);
+  Function *F = cast<Function>(M->getNamedValue("f"));
+  BasicBlock &BB = F->front();
+
+  auto *I1 = BB.getFirstNonPHI();
+  auto *I2 = I1->getNextNode();
+  auto *I3 = BB.getTerminator();
+
+  EXPECT_FALSE(bool(I1->getDebugLoc()));
+  I1->updateLocationAfterHoist();
+  EXPECT_FALSE(bool(I1->getDebugLoc()));
+
+  const MDNode *Scope = I2->getDebugLoc().getScope();
+  EXPECT_EQ(I2->getDebugLoc().getLine(), 2U);
+  I2->updateLocationAfterHoist();
+  EXPECT_EQ(I2->getDebugLoc().getLine(), 0U);
+  EXPECT_EQ(I2->getDebugLoc().getScope(), Scope);
+
+  EXPECT_EQ(I3->getDebugLoc().getLine(), 2U);
+  I3->updateLocationAfterHoist();
+  EXPECT_FALSE(bool(I3->getDebugLoc()));
+}
+
 } // end anonymous namespace
 } // end namespace llvm