[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

   ///     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.
+  /// Note: it is undefined behavior to call this on an instruction not
+  /// currently inserted into a function.
+  void updateLocationAfterHoist();
+
+  /// Drop the instruction's debug location. This does not guarantee removal
+  /// of the !dbg source location attachment, as it must set a line 0 location
+  /// with scope information attached on call instructions. To guarantee
+  /// removal of the !dbg attachment, use the \ref setDebugLoc() API.
+  /// Note: it is undefined behavior to call this on an instruction not
+  /// currently inserted into a function.
+  void dropLocation();
+
 private:
   /// Return true if we have an entry in the on-the-side metadata hash.
   bool hasMetadataHashEntry() const {
     return Bitfield::test<HasMetadataField>(getSubclassDataFromValue());
   }

llvm/lib/IR/DebugInfo.cpp

 void Instruction::applyMergedLocation(const DILocation *LocA,
                                       const DILocation *LocB) {
   setDebugLoc(DILocation::getMergedLocation(LocA, LocB));
 }
 
+void Instruction::updateLocationAfterHoist() { dropLocation(); }
+
+void Instruction::dropLocation() {
+  const DebugLoc &DL = getDebugLoc();
+  if (!DL)
+    return;
+
+  // If this isn't 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 calls to preserve scope information in case
+  // inlining occurs.
+  const DISubprogram *SP = getFunction()->getSubprogram();
+  if (SP)
+    // If a function scope is available, set it on the line 0 location. When
+    // hoisting a call to a predecessor block, using the function scope avoids
+    // making it look like the callee was reached earlier than it should be.
+    setDebugLoc(DebugLoc::get(0, 0, SP));
+  else
+    // The parent function has no scope. Go ahead and drop the location. If
+    // the parent function is inlined, and the callee has a subprogram, the
+    // inliner will attach a location to the call.
+    //
+    // One alternative is to set a line 0 location with the existing scope and
+    // inlinedAt info. The location might be sensitive to when inlining occurs.
+    setDebugLoc(DebugLoc());
+}
+
 //===----------------------------------------------------------------------===//
 // LLVM C API implementations.
 //===----------------------------------------------------------------------===//
 
 static unsigned map_from_llvmDWARFsourcelanguage(LLVMDWARFSourceLanguage lang) {

llvm/lib/Transforms/Scalar/GVN.cpp

 #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"

   // 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.

llvm/lib/Transforms/Scalar/LICM.cpp

     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;

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

 ; }
 ;
 ; 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

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

 
 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

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

llvm/unittests/IR/InstructionsTest.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"

 
   F->deleteValue();
   I->deleteValue();
 }
 
+TEST(InstructionsTest, UpdateLocationAfterHoist) {
+  LLVMContext C;
+  std::unique_ptr<Module> M = parseIR(C,
+                                      R"(
+      declare void @callee()
+
+      define void @no_parent_scope() {
+        call void @callee()           ; I1: Call with no location.
+        call void @callee(), !dbg !11 ; I2: Call with location.
+        ret void, !dbg !11            ; I3: Non-call with location.
+      }
+
+      define void @with_parent_scope() !dbg !8 {
+        call void @callee()           ; I1: Call with no location.
+        call void @callee(), !dbg !11 ; I2: Call with location.
+        ret void, !dbg !11            ; I3: Non-call with location.
+      }
+
+      !llvm.dbg.cu = !{!0}
+      !llvm.module.flags = !{!3, !4}
+      !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "", 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, inlinedAt: !12)
+      !12 = !DILocation(line: 3, column: 8, scope: !8)
+  )");
+  ASSERT_TRUE(M);
+
+  {
+    Function *NoParentScopeF =
+        cast<Function>(M->getNamedValue("no_parent_scope"));
+    BasicBlock &BB = NoParentScopeF->front();
+
+    auto *I1 = BB.getFirstNonPHI();
+    auto *I2 = I1->getNextNode();
+    auto *I3 = BB.getTerminator();
+
+    EXPECT_FALSE(bool(I1->getDebugLoc()));
+    I1->dropLocation();
+    EXPECT_FALSE(bool(I1->getDebugLoc()));
+
+    EXPECT_EQ(I2->getDebugLoc().getLine(), 2U);
+    I2->dropLocation();
+    EXPECT_FALSE(bool(I1->getDebugLoc()));
+
+    EXPECT_EQ(I3->getDebugLoc().getLine(), 2U);
+    I3->dropLocation();
+    EXPECT_FALSE(bool(I3->getDebugLoc()));
+  }
+
+  {
+    Function *WithParentScopeF =
+        cast<Function>(M->getNamedValue("with_parent_scope"));
+    BasicBlock &BB = WithParentScopeF->front();
+
+    auto *I2 = BB.getFirstNonPHI()->getNextNode();
+
+    MDNode *Scope = cast<MDNode>(WithParentScopeF->getSubprogram());
+    EXPECT_EQ(I2->getDebugLoc().getLine(), 2U);
+    I2->dropLocation();
+    EXPECT_EQ(I2->getDebugLoc().getLine(), 0U);
+    EXPECT_EQ(I2->getDebugLoc().getScope(), Scope);
+    EXPECT_EQ(I2->getDebugLoc().getInlinedAt(), nullptr);
+  }
+}
+
 } // end anonymous namespace
 } // end namespace llvm