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

[davide]

Summary

Full analysis in: https://bugs.llvm.org/show_bug.cgi?id=46120
tl;dr: avoid jumpiness in line tables

[probinson]

That's a pretty long test case for a one-line patch; there's no existing zext testcase where you can just add !dbg metadata?

[davide]

Let me try putting an assertion with the patch and see what I can do.

[davide]

Still looking for a reduced tescase but I noticed another bug, so I put another review online:
https://reviews.llvm.org/D81879

[davide]

I tried really hard to reduce this one but I failed. I guess I can either drop this review or I'm happy to hear suggestions. @probinson

[davide]

I might have found a way to reduce :)

[davide]

Updated diff/reduced testcase/found another bug/added context.

[vsk]

llvm/lib/CodeGen/CodeGenPrepare.cpp

5809

Might be worth moving parts of this comment near the TypePromotionTransaction definition, to justify why those clear the IRBuilder debug loc?

llvm/test/DebugInfo/X86/zextload.ll

26

{{$}}?

[davide]

Address Vedant's feedback.

[vsk]

Lgtm with minor nitpicks inline, thanks!

llvm/lib/CodeGen/CodeGenPrepare.cpp

2652

To quibble: s/would/could/ might be more fair, since we're conservatively dropping the location just in case the created instruction gets moved around after the fact -- it's just that this tends to happen a lot :). Also, maybe moving this to the TypePromotionAction class definition would serve as a more general warning?

[probinson]

Definitely better, thanks!

[davide]

Committed

commit 1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9 (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <[e-mail]>
Date:   Wed Jun 17 10:29:47 2020 -0700

    [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

llvm/lib/CodeGen/CodeGenPrepare.cpp

void undo() override {
       LLVM_DEBUG(dbgs() << "Undo: SExtBuilder: " << *Val << "\n");
       if (Instruction *IVal = dyn_cast<Instruction>(Val))
         IVal->eraseFromParent();
     }
   };
 
   /// Build a zero extension instruction.
   class ZExtBuilder : public TypePromotionAction {
     Value *Val;
 
   public:
     /// Build a zero extension instruction of \p Opnd producing a \p Ty
     /// result.
     /// zext Opnd to Ty.
     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");
     }
 
     /// Get the built value.
     Value *getBuiltValue() { return Val; }
 
     /// Remove the built instruction.
bool CodeGenPrepare::optimizeExt(Instruction *&Inst) {
   LoadInst *LI = nullptr;
   Instruction *ExtFedByLoad;
 
   // Try to promote a chain of computation if it allows to form an extended
   // load.
   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;
   }
 
   // Continue promoting SExts if known as considerable depending on targets.
   if (ATPConsiderable &&
       performAddressTypePromotion(Inst, AllowPromotionWithoutCommonHeader,

llvm/test/DebugInfo/X86/zextload.ll

 ;
 ; When CGP moves a zext Z of a load L to the block where L lives,  Z should not
 ; retain its original debug location. Instead, Z should reuse the debug location
 ; associated with L. Logically the zero extend will become part of the load; the
 ; code generator will attempt to fuse the two instructions into a zextload.
 
 ; 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"
 target triple = "x86_64-unknown-linux-gnu"
 
 define i64 @test(i32* %ptr, i32 %cond) !dbg !5 {
 entry:
   %0 = load i32, i32* %ptr, align 4, !dbg !7

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

+; 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)