Fix out-of-order stepping behavior in programs with hoisted constants.

[ormris]

Summary

When the Constant Hoisting pass moves expensive constants into a common block,
it assignes a debug location equal to the last use of that constant. While this
is certainly intuitive, it places the constant in an out-of-order location,
according to the debug location information. This produces out-of-order
stepping when debugging programs effected by this pass.

This patch creates in-order stepping behavior by removing the debug location
information from hoisted constants.

[ormris]

When the Constant Hoisting pass moves expensive constants into a common block,
it assignes a debug location equal to the last use of that constant. While this
is certainly intuitive, it places the constant in an out-of-order location,
according to the debug location information. This produces out-of-order
stepping when debugging programs effected by this pass.

This patch creates in-order stepping behavior by removing the debug location
information from hoisted constants.

[craig.topper]

lib/Transforms/Scalar/ConstantHoisting.cpp

745 ↗

(On Diff #116029)

This code now longer matches the comment. And the asserts are no longer required since they existed to ensure validity of the previous behavior.

[ormris]

Updating D38088: Fix out-of-order stepping behavior in programs with hoisted constants.

Change list:

• Remove unnecessary asserts
• Edit comment to ensure consistency with new behavior

[echristo]

Adding Paul since he's looked at this stuff extensively.

[probinson]

This came out of our sample-PGO work, wasn't sure it would be kosher for me to approve a patch from the guy in the next cube.
It's completely in line with the other similar patches that Andrea and Dehao have been doing. So if Craig is happy, I'm happy.

[echristo]

I mean, I disagree with the general behavior, but I think I've lost that argument so if it's consistent go ahead with it :)

[craig.topper]

I'm not sure I know enough about debug info to approve this myself. I just knew enough to know that the comment didn't match up after the change.

[probinson]

Well then! LGTM.

In D38088#878342, @echristo wrote:

I mean, I disagree with the general behavior, but I think I've lost that argument so if it's consistent go ahead with it :)

I am not super excited about the tactic either, but preserving the line info on instructions that get moved around like this is messing up an important consumer (SPGO) so I'm willing to tolerate it until we can come up with something better.

[ormris]

Great! Thanks @craig.topper for your review comments. Could someone commit this change? I don't have commit access currently.

[ormris]

Updated the pass to use the merge API. This version merges the locations of all uses
of the hoisted constant, as well as the location of the instruction at the insertion
point.

[aprantl]

Paul, can you commit this?

[ormris]

Thanks for the review!

llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp

 #include "llvm/Pass.h"
 #include "llvm/Support/BlockFrequency.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/Local.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <iterator>
 #include <tuple>
 #include <utility>
 
 using namespace llvm;
for (auto const &ConstInfo : ConstantVec) {
     assert(!IPSet.empty() && "IPSet is empty");
 
     unsigned UsesNum = 0;
     unsigned ReBasesNum = 0;
     for (Instruction *IP : IPSet) {
       IntegerType *Ty = ConstInfo.BaseConstant->getType();
       Instruction *Base =
           new BitCastInst(ConstInfo.BaseConstant, Ty, "const", IP);
+
+      Base->setDebugLoc(IP->getDebugLoc());
+
       DEBUG(dbgs() << "Hoist constant (" << *ConstInfo.BaseConstant
                    << ") to BB " << IP->getParent()->getName() << '\n'
                    << *Base << '\n');
 
       // Emit materialization code for all rebased constants.
       unsigned Uses = 0;
       for (auto const &RCI : ConstInfo.RebasedConstants) {
         for (auto const &U : RCI.Uses) {
           Uses++;
           BasicBlock *OrigMatInsertBB =
               findMatInsertPt(U.Inst, U.OpndIdx)->getParent();
           // If Base constant is to be inserted in multiple places,
           // generate rebase for U using the Base dominating U.
           if (IPSet.size() == 1 ||
               DT->dominates(Base->getParent(), OrigMatInsertBB)) {
             emitBaseConstants(Base, RCI.Offset, U);
             ReBasesNum++;
           }
+
+          Base->setDebugLoc(DILocation::getMergedLocation(Base->getDebugLoc(), U.Inst->getDebugLoc()));
         }
       }
       UsesNum = Uses;
 
       // Use the same debug location as the last user of the constant.
       assert(!Base->use_empty() && "The use list is empty!?");
       assert(isa<Instruction>(Base->user_back()) &&
              "All uses should be instructions.");
-      Base->setDebugLoc(cast<Instruction>(Base->user_back())->getDebugLoc());
     }
     (void)UsesNum;
     (void)ReBasesNum;
     // Expect all uses are rebased after rebase is done.
     assert(UsesNum == ReBasesNum && "Not all uses are rebased");
 
     NumConstantsHoisted++;
 

llvm/trunk/test/Transforms/ConstantHoisting/X86/dbg-dominatingblock.ll

+; RUN: opt -S -consthoist < %s | FileCheck %s
+; ModuleID = 'test-hoist-debug.cpp'
+source_filename = "test-hoist-debug.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind optnone uwtable
+define i32 @_Z3foov() !dbg !7 {
+; CHECK: bitcast
+; CHECK-NOT: !dbg !11
+; CHECK: inttoptr 
+entry:
+  %a0 = inttoptr i64 4646526064 to i32*
+  %v0 = load i32, i32* %a0, align 16, !dbg !11
+  %c = alloca i32, align 4
+  store i32 1, i32* %c, align 4
+  %0 = load i32, i32* %c, align 4
+  %cmp = icmp eq i32 %0, 0
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %a1 = inttoptr i64 4646526080 to i32*
+  %v1 = load i32, i32* %a1, align 16, !dbg !11
+  br label %return
+
+if.else:                                          ; preds = %entry
+  %a2 = inttoptr i64 4646526096 to i32*
+  %v2 = load i32, i32* %a2, align 16, !dbg !11
+  br label %return
+
+return:                                           ; preds = %if.else, %if.then
+  %vx = phi i32 [%v1, %if.then], [%v2, %if.else]
+  %r0 = add i32 %vx, %v0
+
+  ret i32 %r0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 313291)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test-hoist-debug.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 (trunk 313291)"}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 2, column: 3, scope: !7)
+!12 = !DILocation(line: 3, column: 3, scope: !7)
+!13 = !DILocation(line: 4, column: 3, scope: !7)

llvm/trunk/test/Transforms/ConstantHoisting/X86/dbg-samebasicblock.ll

+; RUN: opt -S -consthoist < %s | FileCheck %s
+; ModuleID = 'test-hoist-debug.cpp'
+source_filename = "test-hoist-debug.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: noinline nounwind optnone uwtable
+define i32 @_Z3foov() !dbg !7 {
+; CHECK: bitcast
+; CHECK: !dbg !11
+; CHECK: inttoptr 
+  %a0 = inttoptr i64 4646526064 to i32*, !dbg !11
+  %v0 = load i32, i32* %a0, align 16, !dbg !11
+
+  %a1 = inttoptr i64 4646526080 to i32*
+  %v1 = load i32, i32* %a1, align 16, !dbg !11
+
+  %a2 = inttoptr i64 4646526096 to i32*
+  %v2 = load i32, i32* %a2, align 16, !dbg !11
+
+  %r0 = add i32 %v0, %v1
+  %r1 = add i32 %r0, %v2
+  ret i32 %r1
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 313291)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test-hoist-debug.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 (trunk 313291)"}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "_Z3foov", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 2, column: 3, scope: !7)