[DebugInfo][ConstraintElimination] Fix debug value loss in replacing comparisons with the speculated constants #136839
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms Author: Shan Huang (Apochens) ChangesFix #135736 Full diff: https://github.com/llvm/llvm-project/pull/136839.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 58d705eb6aa96..4142c8aa3100d 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -25,6 +25,7 @@
#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/Dominators.h"
+#include "llvm/IR/DebugInfo.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/InstrTypes.h"
@@ -1454,8 +1455,20 @@ static bool checkAndReplaceCondition(
return ShouldReplace;
});
NumCondsRemoved++;
- if (Cmp->use_empty())
+ if (Cmp->use_empty()) {
+ SmallVector<DbgVariableIntrinsic *> DbgUsers;
+ SmallVector<DbgVariableRecord *> DVRUsers;
+ findDbgUsers(DbgUsers, Cmp, &DVRUsers);
+
+ for (auto *DVR: DVRUsers) {
+ auto *DTN = DT.getNode(DVR->getParent());
+ if (!DTN || DTN->getDFSNumIn() < NumIn || DTN->getDFSNumOut() > NumOut)
+ continue;
+ DVR->replaceVariableLocationOp(Cmp, ConstantC);
+ }
+
ToRemove.push_back(Cmp);
+ }
return Changed;
};
diff --git a/llvm/test/Transforms/ConstraintElimination/salvage-dbg-values-replaced-by-constant.ll b/llvm/test/Transforms/ConstraintElimination/salvage-dbg-values-replaced-by-constant.ll
new file mode 100644
index 0000000000000..75fae6f93992b
--- /dev/null
+++ b/llvm/test/Transforms/ConstraintElimination/salvage-dbg-values-replaced-by-constant.ll
@@ -0,0 +1,80 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=constraint-elimination -S | FileCheck %s
+
+; Check that checkAndReplaceCondition() salvages the debug value information after replacing
+; the conditions (%t.1 in this test) with the speculated constants (GitHub Issue #135736).
+
+define i1 @test_and_ule(i4 %x, i4 %y, i4 %z) !dbg !5 {
+; CHECK-LABEL: define i1 @test_and_ule(
+; CHECK-SAME: i4 [[X:%.*]], i4 [[Y:%.*]], i4 [[Z:%.*]]) !dbg [[DBG5:![0-9]+]] {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[C_1:%.*]] = icmp ule i4 [[X]], [[Y]], !dbg [[DBG11:![0-9]+]]
+; CHECK-NEXT: [[C_2:%.*]] = icmp ule i4 [[Y]], [[Z]], !dbg [[DBG12:![0-9]+]]
+; CHECK-NEXT: [[AND:%.*]] = and i1 [[C_1]], [[C_2]], !dbg [[DBG13:![0-9]+]]
+; CHECK-NEXT: br i1 [[AND]], label %[[THEN:.*]], label %[[EXIT:.*]], !dbg [[DBG14:![0-9]+]]
+; CHECK: [[THEN]]:
+; CHECK-NEXT: #dbg_value(i1 true, [[META9:![0-9]+]], !DIExpression(), [[META15:![0-9]+]])
+; CHECK-NEXT: [[R_1:%.*]] = xor i1 true, true, !dbg [[DBG16:![0-9]+]]
+; CHECK-NEXT: br label %[[EXIT]]
+; CHECK: [[EXIT]]:
+; CHECK-NEXT: #dbg_value(i1 poison, [[META17:![0-9]+]], !DIExpression(), [[META15]])
+; CHECK-NEXT: ret i1 true, !dbg [[DBG18:![0-9]+]]
+;
+entry:
+ %c.1 = icmp ule i4 %x, %y, !dbg !11
+ %c.2 = icmp ule i4 %y, %z, !dbg !12
+ %t.1 = icmp ule i4 %x, %z, !dbg !13
+ %and = and i1 %c.1, %c.2, !dbg !14
+ br i1 %and, label %then, label %exit, !dbg !15
+
+then: ; preds = %entry
+ #dbg_value(i1 %t.1, !9, !DIExpression(), !13)
+ %r.1 = xor i1 %t.1, %t.1, !dbg !16
+ br label %exit
+
+exit: ; preds = %bb1, %entry
+ #dbg_value(i1 %t.1, !17, !DIExpression(), !13)
+ ret i1 true, !dbg !18
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.debugify = !{!2, !3}
+!llvm.module.flags = !{!4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "temp.ll", directory: "/")
+!2 = !{i32 20}
+!3 = !{i32 17}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "test_and_ule", linkageName: "test_and_ule", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9}
+!9 = !DILocalVariable(name: "4", scope: !5, file: !1, line: 5, type: !10)
+!10 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+!11 = !DILocation(line: 1, column: 1, scope: !5)
+!12 = !DILocation(line: 2, column: 1, scope: !5)
+!13 = !DILocation(line: 5, column: 1, scope: !5)
+!14 = !DILocation(line: 3, column: 1, scope: !5)
+!15 = !DILocation(line: 4, column: 1, scope: !5)
+!16 = !DILocation(line: 7, column: 1, scope: !5)
+!17 = !DILocalVariable(name: "6", scope: !5, file: !1, line: 7, type: !10)
+!18 = !DILocation(line: 20, column: 1, scope: !5)
+;.
+; CHECK: [[META0:![0-9]+]] = distinct !DICompileUnit(language: DW_LANG_C, file: [[META1:![0-9]+]], producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+; CHECK: [[META1]] = !DIFile(filename: "temp.ll", directory: {{.*}})
+; CHECK: [[DBG5]] = distinct !DISubprogram(name: "test_and_ule", linkageName: "test_and_ule", scope: null, file: [[META1]], line: 1, type: [[META6:![0-9]+]], scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: [[META0]], retainedNodes: [[META8:![0-9]+]])
+; CHECK: [[META6]] = !DISubroutineType(types: [[META7:![0-9]+]])
+; CHECK: [[META7]] = !{}
+; CHECK: [[META8]] = !{[[META9]]}
+; CHECK: [[META9]] = !DILocalVariable(name: "4", scope: [[DBG5]], file: [[META1]], line: 5, type: [[META10:![0-9]+]])
+; CHECK: [[META10]] = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+; CHECK: [[DBG11]] = !DILocation(line: 1, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG12]] = !DILocation(line: 2, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG13]] = !DILocation(line: 3, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG14]] = !DILocation(line: 4, column: 1, scope: [[DBG5]])
+; CHECK: [[META15]] = !DILocation(line: 5, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG16]] = !DILocation(line: 7, column: 1, scope: [[DBG5]])
+; CHECK: [[META17]] = !DILocalVariable(name: "6", scope: [[DBG5]], file: [[META1]], line: 7, type: [[META10]])
+; CHECK: [[DBG18]] = !DILocation(line: 20, column: 1, scope: [[DBG5]])
+;.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
OCHyams
reviewed
Apr 23, 2025
| @@ -0,0 +1,80 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | |||
There was a problem hiding this comment.
Sometimes update_test_checks isn't appropriate for dbg tests (can be hard to understand exactly what's being tested, making incorrect future updates using the script more likely to slip in), but in this case I think the test case is simple enough that it's ok.
There was a problem hiding this comment.
Thanks for the reminder. In the latest commit, I have made the test checks more concise.
| auto *DTN = DT.getNode(DVR->getParent()); | ||
| if (!DTN || DTN->getDFSNumIn() < NumIn || DTN->getDFSNumOut() > NumOut) | ||
| continue; | ||
| DVR->replaceVariableLocationOp(Cmp, ConstantC); |
There was a problem hiding this comment.
The non-debug code also has a check that if the user comes before the ContextInst it's skipped. I think we probably need to copy that too?
; ModuleID = '/app/example.ll'
source_filename = "/app/example.ll"
; Function Attrs: nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write)
declare void @llvm.assume(i1 noundef) #0
declare void @may_unwind()
declare void @use(i1)
define i1 @assume_single_bb_conditions_after_assume(i8 %a, i8 %b, i1 %c) !dbg !5 {
%add.1 = add nuw nsw i8 %a, 1, !dbg !11
%cmp.1 = icmp ule i8 %add.1, %b, !dbg !12
%c.1 = icmp ule i8 %add.1, %b, !dbg !13
#dbg_value(i1 %c.1, !9, !DIExpression(), !14)
call void @use(i1 %c.1), !dbg !15
#dbg_value(i1 %c.1, !9, !DIExpression(), !14)
call void @may_unwind(), !dbg !16
#dbg_value(i1 %c.1, !9, !DIExpression(), !14)
call void @llvm.assume(i1 %cmp.1), !dbg !17
#dbg_value(i1 %c.1, !9, !DIExpression(), !14)
%t.2 = icmp ule i8 %a, %b, !dbg !14
#dbg_value(i1 %c.1, !9, !DIExpression(), !14)
%res.1 = xor i1 %c.1, %t.2, !dbg !18
%add.2 = add nuw nsw i8 %a, 2, !dbg !19
%c.2 = icmp ule i8 %add.2, %b, !dbg !20
%res.2 = xor i1 %res.1, %c.2, !dbg !21
ret i1 %res.2, !dbg !22
}
attributes #0 = { nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }
!llvm.dbg.cu = !{!0}
!llvm.debugify = !{!2, !3}
!llvm.module.flags = !{!4}
!0 = distinct !DICompileUnit(language: DW_LANG_C, file: !1, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
!1 = !DIFile(filename: "/app/example.ll", directory: "/")
!2 = !{i32 12}
!3 = !{i32 8}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = distinct !DISubprogram(name: "assume_single_bb_conditions_after_assume", linkageName: "assume_single_bb_conditions_after_assume", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
!6 = !DISubroutineType(types: !7)
!7 = !{}
!8 = !{!9}
!9 = !DILocalVariable(name: "4", scope: !5, file: !1, line: 7, type: !10)
!10 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
!11 = !DILocation(line: 1, column: 1, scope: !5)
!12 = !DILocation(line: 2, column: 1, scope: !5)
!13 = !DILocation(line: 3, column: 1, scope: !5)
!14 = !DILocation(line: 7, column: 1, scope: !5)
!15 = !DILocation(line: 4, column: 1, scope: !5)
!16 = !DILocation(line: 5, column: 1, scope: !5)
!17 = !DILocation(line: 6, column: 1, scope: !5)
!18 = !DILocation(line: 8, column: 1, scope: !5)
!19 = !DILocation(line: 9, column: 1, scope: !5)
!20 = !DILocation(line: 10, column: 1, scope: !5)
!21 = !DILocation(line: 11, column: 1, scope: !5)
!22 = !DILocation(line: 12, column: 1, scope: !5)
I think it's probably incorrect for the dbg_values to have their use replaced with true before the assume. Annoyingly, it looks like we won't be able to catch all dbg_values though (the 3rd one doesn't get updated because it comes before the ContextInst in that case... not sure there's much we can do about that?). IMO adding a comment with a FIXME noting the shortcoming is good enough here, if we can't think of something better.
(Can you add this as another regression test please?)
There was a problem hiding this comment.
I have added this as another regression test salvage-dbg-values-replaced-by-constant-2.ll. However, the optimized IR shows that the forth dbg_value cannot be updated as well, because the ContextInst is %t.2.
| @@ -1454,8 +1455,20 @@ static bool checkAndReplaceCondition( | |||
| return ShouldReplace; | |||
| }); | |||
| NumCondsRemoved++; | |||
| if (Cmp->use_empty()) | |||
| if (Cmp->use_empty()) { | |||
There was a problem hiding this comment.
Hmmm.. .I think we probably want to do the debug update outside of the if (Cmp->use_empty()) block (unconditionally)? Otherwise we're only updating debug info if there are no other non-dbg uses. But IMO we should update the debug info regardless, because we know the value on this code path.
(I'm not sure there's a clear correct answer, happy to discuss further).
There was a problem hiding this comment.
🤔 It seems that this is the right way. The condition in replaceUesWithIf specifies where we can use the speculated constant. As a result, although there could be cases that non-dbg uses still exist after the optimization, we are able to update the dbg_values that satisfy the replaceUesWithIf condition. I have modified the return value to %t.1 in test salvage-dbg-values-replaced-by-constant.ll to demostrate such a case.
OCHyams
approved these changes
Apr 29, 2025
Comment on lines
5
to
6
| ; In particular, the debug value record uses are replaced if their `DFSNumIn`s and `DFSNumOut`s | ||
| ; on `DominatorTree` satisfy the contraint. |
There was a problem hiding this comment.
This comment feels slightly awkward to parse - perhaps something like "In particular, debug uses are replaced if the debug record is dominated by the condition fact" (@fhahn might be able to produce better wording?)
Comment on lines
6
to
7
| ; In particular, the debug value record uses should not be replaced if they comes before the | ||
| ; context instrtuction (e.g., `%t.2` in this example). |
Comment on lines
+1465
to
+1473
| for (auto *DVR : DVRUsers) { | ||
| auto *DTN = DT.getNode(DVR->getParent()); | ||
| if (!DTN || DTN->getDFSNumIn() < NumIn || DTN->getDFSNumOut() > NumOut) | ||
| continue; | ||
|
|
||
| auto *MarkedI = DVR->getInstruction(); | ||
| if (MarkedI->getParent() == ContextInst->getParent() && | ||
| MarkedI->comesBefore(ContextInst)) | ||
| continue; |
There was a problem hiding this comment.
Hmm, part of me wants to reduce code duplication by introducing a lambda that is used by this code and the checks in replaceUsesWithIf above. That gives an added benefit of naming what it's doing. On the other hand, that might be more boilerplate than is needed here, so I'm happy for it to stay as it is too.
|
(Please also make the clang-format bot happy) |
OCHyams
reviewed
Apr 30, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/94/builds/6836 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…comparisons with the speculated constants (llvm#136839) Fix llvm#135736
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…comparisons with the speculated constants (llvm#136839) Fix llvm#135736
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…comparisons with the speculated constants (llvm#136839) Fix llvm#135736
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…comparisons with the speculated constants (llvm#136839) Fix llvm#135736
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #135736