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][RemoveDIs] Maintain DPValues on skipped instrs in CGP #74602

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

jmorse
Copy link
Member

@jmorse jmorse commented Dec 6, 2023

It turns out that CodeGenPrepare will skip over consecutive select instructions as it knows it can optimise them all at the same time. This is unfortunate for the RemoveDIs project to remove intrinsic-based debug-info, because that means debug-info attached to those skipped instructions doesn't get seen by optimizeInst and so updated. Add code to handle debug-info on those skipped instructions manually.

This code will also have been slower when it had dbg.values stuffed in between instructions, but with RemoveDIs it'll go faster because the dbg.values won't break up the select sequence.

It turns out that CodeGenPrepare will skip over consecutive select
instructions as it knows it can optimise them all at the same time. This is
unfortunate for the RemoveDIs project to remove intrinsic-based debug-info,
because that means debug-info attached to those skipped instructions
doesn't get seen by optimizeInst and so updated. Add code to handle
debug-info on those skipped instructions manually.

This code will also have been slower when it had dbg.values stuffed in
between instructions, but with RemoveDIs it'll go faster because the
dbg.values won't break up the select sequence.
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 6, 2023

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

It turns out that CodeGenPrepare will skip over consecutive select instructions as it knows it can optimise them all at the same time. This is unfortunate for the RemoveDIs project to remove intrinsic-based debug-info, because that means debug-info attached to those skipped instructions doesn't get seen by optimizeInst and so updated. Add code to handle debug-info on those skipped instructions manually.

This code will also have been slower when it had dbg.values stuffed in between instructions, but with RemoveDIs it'll go faster because the dbg.values won't break up the select sequence.


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+14-2)
  • (added) llvm/test/Transforms/CodeGenPrepare/debug-info-on-skipped-selects.ll (+67)
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 824371c9b9f91..8565a808de30b 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -460,6 +460,7 @@ class CodeGenPrepare : public FunctionPass {
   bool dupRetToEnableTailCallOpts(BasicBlock *BB, ModifyDT &ModifiedDT);
   bool fixupDbgValue(Instruction *I);
   bool fixupDPValue(DPValue &I);
+  bool fixupDPValuesOnInst(Instruction &I);
   bool placeDbgValues(Function &F);
   bool placePseudoProbes(Function &F);
   bool canFormExtLd(const SmallVectorImpl<Instruction *> &MovedExts,
@@ -6986,6 +6987,11 @@ bool CodeGenPrepare::optimizeSelectInst(SelectInst *SI) {
   // Increment the current iterator to skip all the rest of select instructions
   // because they will be either "not lowered" or "all lowered" to branch.
   CurInstIterator = std::next(LastSI->getIterator());
+  // Examine any debug-info attached to those instructions, which won't be seen
+  // elsewhere. But only do it once we return and have potentially rewritten
+  // valeus.
+  for (SelectInst *SI : ArrayRef(ASI).drop_front())
+    fixupDPValuesOnInst(*SI);
 
   bool VectorCond = !SI->getCondition()->getType()->isIntegerTy(1);
 
@@ -8141,8 +8147,7 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
 
 bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
   bool AnyChange = false;
-  for (DPValue &DPV : I->getDbgValueRange())
-    AnyChange |= fixupDPValue(DPV);
+  AnyChange = fixupDPValuesOnInst(*I);
 
   // Bail out if we inserted the instruction to prevent optimizations from
   // stepping on each other's toes.
@@ -8408,6 +8413,13 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
   return AnyChange;
 }
 
+bool CodeGenPrepare::fixupDPValuesOnInst(Instruction &I) {
+  bool AnyChange = false;
+  for (DPValue &DPV : I.getDbgValueRange())
+    AnyChange |= fixupDPValue(DPV);
+  return AnyChange;
+}
+
 // FIXME: should updating debug-info really cause the "changed" flag to fire,
 // which can cause a function to be reprocessed?
 bool CodeGenPrepare::fixupDPValue(DPValue &DPV) {
diff --git a/llvm/test/Transforms/CodeGenPrepare/debug-info-on-skipped-selects.ll b/llvm/test/Transforms/CodeGenPrepare/debug-info-on-skipped-selects.ll
new file mode 100644
index 0000000000000..f2b11207afc7b
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/debug-info-on-skipped-selects.ll
@@ -0,0 +1,67 @@
+; RUN: llc %s -stop-after=codegenprepare -o - | FileCheck %s
+; RUN: llc %s -stop-after=codegenprepare -o - --try-experimental-debuginfo-iterators | FileCheck %s
+;
+; Test that when we skip over multiple selects in CGP, that the debug-info
+; attached to those selects is still fixed up.
+
+; CHECK: declare void @llvm.dbg.value(metadata,
+; CHECK: call void @llvm.dbg.value(metadata ptr %sunkaddr,
+
+source_filename = "reduced.ll"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%"class.(anonymous namespace)::CFIInstrInserter" = type { ptr, ptr, ptr, ptr }
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+define i1 @_ZN12_GLOBAL__N_116CFIInstrInserter20runOnMachineFunctionERN4llvm15MachineFunctionE(ptr %this, i1 %or.cond.i) !dbg !5 {
+entry:
+  %CSRLocMap.i = getelementptr %"class.(anonymous namespace)::CFIInstrInserter", ptr %this, i64 0, i32 2, !dbg !16
+  %bf.load.i.i.i.i = load i32, ptr %CSRLocMap.i, align 8, !dbg !17
+  br i1 %or.cond.i, label %_ZN4llvm12DenseMapBaseINS_13SmallDenseMapIjN12_GLOBAL__N_116CFIInstrInserter16CSRSavedLocationELj16ENS_12DenseMapInfoIjvEENS_6detail12DenseMapPairIjS4_EEEEjS4_S6_S9_E5clearEv.exit.i, label %if.end.i.i, !dbg !18
+
+if.end.i.i:                                       ; preds = %entry
+  store ptr null, ptr null, align 8, !dbg !19
+  %bf.load.i.i.i.pre.i.i.i.i.i = load i32, ptr %CSRLocMap.i, align 8, !dbg !20
+  %cond.i.i.i.i.i.i.i.i.i = select i1 false, ptr null, ptr null, !dbg !21
+  tail call void @llvm.dbg.value(metadata ptr %CSRLocMap.i, metadata !14, metadata !DIExpression()), !dbg !21
+  %cond.i.i.i7.i.i.i.i.i.i = select i1 false, i32 0, i32 0, !dbg !22
+  br label %_ZN4llvm12DenseMapBaseINS_13SmallDenseMapIjN12_GLOBAL__N_116CFIInstrInserter16CSRSavedLocationELj16ENS_12DenseMapInfoIjvEENS_6detail12DenseMapPairIjS4_EEEEjS4_S6_S9_E5clearEv.exit.i, !dbg !23
+
+_ZN4llvm12DenseMapBaseINS_13SmallDenseMapIjN12_GLOBAL__N_116CFIInstrInserter16CSRSavedLocationELj16ENS_12DenseMapInfoIjvEENS_6detail12DenseMapPairIjS4_EEEEjS4_S6_S9_E5clearEv.exit.i: ; preds = %if.end.i.i, %entry
+  ret i1 false, !dbg !24
+}
+
+attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+!llvm.debugify = !{!3, !4}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!2 = !DIFile(filename: "reduced.ll", directory: "/")
+!3 = !{i32 9}
+!4 = !{i32 5}
+!5 = distinct !DISubprogram(name: "_ZN12_GLOBAL__N_116CFIInstrInserter20runOnMachineFunctionERN4llvm15MachineFunctionE", linkageName: "_ZN12_GLOBAL__N_116CFIInstrInserter20runOnMachineFunctionERN4llvm15MachineFunctionE", scope: null, file: !2, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9, !11, !13, !14, !15}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !2, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !5, file: !2, line: 2, type: !12)
+!12 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!13 = !DILocalVariable(name: "3", scope: !5, file: !2, line: 5, type: !12)
+!14 = !DILocalVariable(name: "4", scope: !5, file: !2, line: 6, type: !10)
+!15 = !DILocalVariable(name: "5", scope: !5, file: !2, line: 7, type: !12)
+!16 = !DILocation(line: 1, column: 1, scope: !5)
+!17 = !DILocation(line: 2, column: 1, scope: !5)
+!18 = !DILocation(line: 3, column: 1, scope: !5)
+!19 = !DILocation(line: 4, column: 1, scope: !5)
+!20 = !DILocation(line: 5, column: 1, scope: !5)
+!21 = !DILocation(line: 6, column: 1, scope: !5)
+!22 = !DILocation(line: 7, column: 1, scope: !5)
+!23 = !DILocation(line: 8, column: 1, scope: !5)
+!24 = !DILocation(line: 9, column: 1, scope: !5)

Copy link
Contributor

@OCHyams OCHyams left a comment

Choose a reason for hiding this comment

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

LGTM with small nit

// Examine any debug-info attached to those instructions, which won't be seen
// elsewhere. But only do it once we return and have potentially rewritten
// valeus.
for (SelectInst *SI : ArrayRef(ASI).drop_front())
Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

But only do it once we return and have potentially rewritten valeus.

nit: typo, and also I am not 100% sure what "once we return" means in this context - is there any way to rewrite this slightly?

Copy link
Contributor

Choose a reason for hiding this comment

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

(new version sounds good, thanks)

@jmorse jmorse merged commit d0858bf into llvm:main Dec 6, 2023
3 of 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.

None yet

3 participants