Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Debuginfo][GVNHoist] Fix #86227: update the debug location of the hoisted GEP #86236

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

Apochens
Copy link
Contributor

This PR fixes #86227.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 22, 2024

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

This PR fixes #86227.


Full diff: https://github.com/llvm/llvm-project/pull/86236.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/GVNHoist.cpp (+8)
  • (added) llvm/test/Transforms/GVNHoist/hoist-merge-geps.ll (+66)
diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
index b564f00eb9d166..b95421d2035b58 100644
--- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -951,6 +951,14 @@ void GVNHoist::makeGepsAvailable(Instruction *Repl, BasicBlock *HoistPt,
       OtherGep = cast<GetElementPtrInst>(
           cast<StoreInst>(OtherInst)->getPointerOperand());
     ClonedGep->andIRFlags(OtherGep);
+    
+    // Merge debug locations of GEPs, because the hoisted GEP replaces those
+    // in branches. When cloning, ClonedGep preserves the debug location of 
+    // Gepd, so Gep is skipped to avoid merging it twice.
+    if (OtherGep != Gep) {
+      ClonedGep->applyMergedLocation(
+        ClonedGep->getDebugLoc(), OtherGep->getDebugLoc());
+    }
   }
 
   // Replace uses of Gep with ClonedGep in Repl.
diff --git a/llvm/test/Transforms/GVNHoist/hoist-merge-geps.ll b/llvm/test/Transforms/GVNHoist/hoist-merge-geps.ll
new file mode 100644
index 00000000000000..045b3c2c78280d
--- /dev/null
+++ b/llvm/test/Transforms/GVNHoist/hoist-merge-geps.ll
@@ -0,0 +1,66 @@
+; RUN: opt -S -passes=gvn-hoist < %s | FileCheck %s 
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local void @func(i32 noundef %a, ptr noundef %b) #0 !dbg !10 {
+; Check the merged debug location of hoisted GEP
+; CHECK: entry
+; CHECK: %{{[a-zA-Z0-9_]*}} = getelementptr {{.*}} !dbg [[MERGED_DL:![0-9]+]]
+; CHECK: [[MERGED_DL]] = !DILocation(line: 0, scope: !{{[0-9]+}})
+entry:
+  tail call void @llvm.dbg.value(metadata i32 %a, metadata !16, metadata !DIExpression()), !dbg !17
+  tail call void @llvm.dbg.value(metadata ptr %b, metadata !18, metadata !DIExpression()), !dbg !17
+  %tobool = icmp ne i32 %a, 0, !dbg !19
+  br i1 %tobool, label %if.then, label %if.else, !dbg !21
+
+if.then:                                          ; preds = %entry
+  %arrayidx = getelementptr inbounds i32, ptr %b, i64 1, !dbg !22
+  store i32 1, ptr %arrayidx, align 4, !dbg !24
+  br label %if.end, !dbg !25
+
+if.else:                                          ; preds = %entry
+  %arrayidx1 = getelementptr inbounds i32, ptr %b, i64 1, !dbg !26
+  store i32 1, ptr %arrayidx1, align 4, !dbg !28
+  br label %if.end
+
+if.end:                                           ; preds = %if.else, %if.then
+  ret void, !dbg !29
+}
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 19.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "main.c", directory: "/root/llvm-test/GVNHoist")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!6 = !{i32 7, !"PIE Level", i32 2}
+!7 = !{i32 7, !"uwtable", i32 2}
+!8 = !{i32 7, !"frame-pointer", i32 2}
+!10 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !15)
+!11 = !DISubroutineType(types: !12)
+!12 = !{null, !13, !14}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
+!15 = !{}
+!16 = !DILocalVariable(name: "a", arg: 1, scope: !10, file: !1, line: 1, type: !13)
+!17 = !DILocation(line: 0, scope: !10)
+!18 = !DILocalVariable(name: "b", arg: 2, scope: !10, file: !1, line: 1, type: !14)
+!19 = !DILocation(line: 2, column: 9, scope: !20)
+!20 = distinct !DILexicalBlock(scope: !10, file: !1, line: 2, column: 9)
+!21 = !DILocation(line: 2, column: 9, scope: !10)
+!22 = !DILocation(line: 3, column: 9, scope: !23)
+!23 = distinct !DILexicalBlock(scope: !20, file: !1, line: 2, column: 12)
+!24 = !DILocation(line: 3, column: 14, scope: !23)
+!25 = !DILocation(line: 4, column: 5, scope: !23)
+!26 = !DILocation(line: 5, column: 9, scope: !27)
+!27 = distinct !DILexicalBlock(scope: !20, file: !1, line: 4, column: 12)
+!28 = !DILocation(line: 5, column: 14, scope: !27)
+!29 = !DILocation(line: 7, column: 1, scope: !10)

Copy link

github-actions bot commented Mar 22, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@Apochens
Copy link
Contributor Author

@jmorse @SLTozer @dwblaikie Please review this.

Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change and test LGTM, with some minor nits for the test, and also clang-format is needed (you can see the suggested changes from the PR checks here).

@@ -0,0 +1,66 @@
; RUN: opt -S -passes=gvn-hoist < %s | FileCheck %s

; Function Attrs: noinline nounwind uwtable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits, we can delete the ; Function Attrs lines and the attribute references (#0 and #1).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. I fixed these minor issues, and now the test would be better.

Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good!

Copy link

✅ With the latest revision this PR passed the Python code formatter.

@Apochens
Copy link
Contributor Author

Ping!

@dtcxzyw dtcxzyw merged commit 79ba323 into llvm:main Mar 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Debuginfo][GVNHoist] The hoisted GEP preserves the debug location from one of the conditional branches
5 participants