From 1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9 Mon Sep 17 00:00:00 2001 From: Davide Italiano Date: Wed, 17 Jun 2020 10:29:47 -0700 Subject: [PATCH] [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 | 14 +++----- llvm/test/DebugInfo/X86/zextload.ll | 2 +- .../CodeGenPrepare/X86/promoted-zext-debugloc.ll | 37 ++++++++++++++++++++++ 3 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 llvm/test/Transforms/CodeGenPrepare/X86/promoted-zext-debugloc.ll diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp index 935fc95..f01a1fe 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 d076fa9..7a48c54 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 0000000..f76e101 --- /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) -- 2.7.4