[llvm] 1cbaf84 - [CGP] Reset the debug location when promoting zext(s).

[Davide Italiano via llvm-commits]

Author: Davide Italiano
Date: 2020-06-17T11:13:13-07:00
New Revision: 1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9

URL: https://github.com/llvm/llvm-project/commit/1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9
DIFF: https://github.com/llvm/llvm-project/commit/1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9.diff

LOG: [CGP] Reset the debug location when promoting zext(s).

When the zext gets promoted, it used to retain the original location,
which pessimizes the debugging experience causing an unexpected
jump in stepping at -Og.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46120 (which also
contains a full C repro).

Differential Revision:  https://reviews.llvm.org/D81437

Added: 
    llvm/test/Transforms/CodeGenPrepare/X86/promoted-zext-debugloc.ll

Modified: 
    llvm/lib/CodeGen/CodeGenPrepare.cpp
    llvm/test/DebugInfo/X86/zextload.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 935fc95fee79..f01a1fe65ae6 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -2443,6 +2443,9 @@ namespace {
 /// This class provides transaction based operation on the IR.
 /// Every change made through this class is recorded in the internal state and
 /// can be undone (rollback) until commit is called.
+/// CGP does not check if instructions could be speculatively executed when
+/// moved. Preserving the original location would pessimize the debugging
+/// experience, as well as negatively impact the quality of sample PGO.
 class TypePromotionTransaction {
   /// This represents the common interface of the individual transaction.
   /// Each class implements the logic for doing one specific modification on
@@ -2658,6 +2661,7 @@ class TypePromotionTransaction {
     ZExtBuilder(Instruction *InsertPt, Value *Opnd, Type *Ty)
         : TypePromotionAction(InsertPt) {
       IRBuilder<> Builder(InsertPt);
+      Builder.SetCurrentDebugLocation(DebugLoc());
       Val = Builder.CreateZExt(Opnd, Ty, "promoted");
       LLVM_DEBUG(dbgs() << "Do: ZExtBuilder: " << *Val << "\n");
     }
@@ -5798,16 +5802,8 @@ bool CodeGenPrepare::optimizeExt(Instruction *&Inst) {
   if (canFormExtLd(SpeculativelyMovedExts, LI, ExtFedByLoad, HasPromoted)) {
     assert(LI && ExtFedByLoad && "Expect a valid load and extension");
     TPT.commit();
-    // Move the extend into the same block as the load
+    // Move the extend into the same block as the load.
     ExtFedByLoad->moveAfter(LI);
-    // CGP does not check if the zext would be speculatively executed when moved
-    // to the same basic block as the load. Preserving its original location
-    // would pessimize the debugging experience, as well as negatively impact
-    // the quality of sample pgo. We don't want to use "line 0" as that has a
-    // size cost in the line-table section and logically the zext can be seen as
-    // part of the load. Therefore we conservatively reuse the same debug
-    // location for the load and the zext.
-    ExtFedByLoad->setDebugLoc(LI->getDebugLoc());
     ++NumExtsMoved;
     Inst = ExtFedByLoad;
     return true;

diff  --git a/llvm/test/DebugInfo/X86/zextload.ll b/llvm/test/DebugInfo/X86/zextload.ll
index d076fa97d4b0..7a48c546db57 100644
--- a/llvm/test/DebugInfo/X86/zextload.ll
+++ b/llvm/test/DebugInfo/X86/zextload.ll
@@ -23,7 +23,7 @@
 
 ; CHECK-LABEL: @test
 ; CHECK:   [[LOADVAL:%[0-9]+]] = load i32, i32* %ptr, align 4, !dbg [[DEBUGLOC:![0-9]+]]
-; CHECK-NEXT:                    zext i32 [[LOADVAL]] to i64, !dbg [[DEBUGLOC]]
+; CHECK-NEXT:                    zext i32 [[LOADVAL]] to i64
 ; CHECK:   [[DEBUGLOC]] = !DILocation(line: 3
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

diff  --git a/llvm/test/Transforms/CodeGenPrepare/X86/promoted-zext-debugloc.ll b/llvm/test/Transforms/CodeGenPrepare/X86/promoted-zext-debugloc.ll
new file mode 100644
index 000000000000..f76e101868f0
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/promoted-zext-debugloc.ll
@@ -0,0 +1,37 @@
+; RUN: opt < %s -codegenprepare -S -mtriple=x86_64-unknown-unknown | FileCheck %s --match-full-lines
+  
+; Make sure the promoted zext doesn't get a debug location associated.
+; CHECK: %promoted = zext i8 %t to i64
+
+define void @patatino(i8* %p, i64* %q, i32 %b, i32* %addr) !dbg !6 {
+entry:
+  %t = load i8, i8* %p, align 1, !dbg !8
+  %zextt = zext i8 %t to i32, !dbg !9
+  %add = add nuw i32 %zextt, %b, !dbg !10
+  %add2 = add nuw i32 %zextt, 12, !dbg !11
+  store i32 %add, i32* %addr, align 4, !dbg !12
+  %s = zext i32 %add2 to i64, !dbg !13
+  store i64 %s, i64* %q, align 4, !dbg !14
+  ret void, !dbg !15
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!3, !4}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "a2.ll", directory: "/")
+!2 = !{}
+!3 = !{i32 8}
+!4 = !{i32 0}
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "patatino", linkageName: "patatino", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !2)
+!8 = !DILocation(line: 1, column: 1, scope: !6)
+!9 = !DILocation(line: 2, column: 1, scope: !6)
+!10 = !DILocation(line: 3, column: 1, scope: !6)
+!11 = !DILocation(line: 4, column: 1, scope: !6)
+!12 = !DILocation(line: 5, column: 1, scope: !6)
+!13 = !DILocation(line: 6, column: 1, scope: !6)
+!14 = !DILocation(line: 7, column: 1, scope: !6)
+!15 = !DILocation(line: 8, column: 1, scope: !6)