Skip to content

[DebugInfo][ConstraintElimination] Fix debug value loss in replacing comparisons with the speculated constants #136839

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 5, 2025
Merged

[DebugInfo][ConstraintElimination] Fix debug value loss in replacing comparisons with the speculated constants #136839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af4944f
Fix #135736: salvage debug values with replaced constants
Apochens Apr 23, 2025
4625651
fix clang-tidy
Apochens Apr 23, 2025
aab3828
salvaging debug values outside the if condition and refine tests
Apochens Apr 24, 2025
598197b
test nits
Apochens Apr 30, 2025
3a04fe1
nit
Apochens Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1454,8 +1455,20 @@ static bool checkAndReplaceCondition(
return ShouldReplace;
});
NumCondsRemoved++;
if (Cmp->use_empty())
if (Cmp->use_empty()) {
Copy link
Contributor

@OCHyams OCHyams Apr 23, 2025

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@Apochens Apochens Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 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.

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);
Copy link
Contributor

@OCHyams OCHyams Apr 23, 2025

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

@Apochens Apochens Apr 24, 2025

Choose a reason for hiding this comment

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

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.

}

ToRemove.push_back(Cmp);
}
return Changed;
};

Expand Down
80 changes: 80 additions & 0 deletions llvm/test/Transforms/ConstraintElimination/salvage-dbg-values-replaced-by-constant.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
Copy link
Contributor

@OCHyams OCHyams Apr 23, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Apochens Apochens Apr 24, 2025

Choose a reason for hiding this comment

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

Thanks for the reminder. In the latest commit, I have made the test checks more concise.

; 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]])
;.
Loading