[DebugInfo][NaryReassociate] Fix missing debug location updates #92545

Apochens · 2024-05-17T13:47:53Z

Fix #92537 .

preserving-debugloc-add-mul.ll tests both NaryReassociate-L514 and -L517 fixes.

llvmbot · 2024-05-17T13:48:25Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

Fix #92537 .

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/NaryReassociate.cpp (+2)
(added) llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll (+88)

diff --git a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
index 308622615332f..a3f0889f2b356 100644
--- a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
@@ -512,9 +512,11 @@ Instruction *NaryReassociatePass::tryReassociatedBinaryOp(const SCEV *LHSExpr,
   switch (I->getOpcode()) {
   case Instruction::Add:
     NewI = BinaryOperator::CreateAdd(LHS, RHS, "", I->getIterator());
+    NewI->setDebugLoc(I->getDebugLoc());
     break;
   case Instruction::Mul:
     NewI = BinaryOperator::CreateMul(LHS, RHS, "", I->getIterator());
+    NewI->setDebugLoc(I->getDebugLoc());
     break;
   default:
     llvm_unreachable("Unexpected instruction.");
diff --git a/llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll b/llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll
new file mode 100644
index 0000000000000..f2f2085f833ba
--- /dev/null
+++ b/llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll
@@ -0,0 +1,88 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=nary-reassociate -S | FileCheck %s
+
+; Test that NaryReassociate's tryReassociatedBinaryOp() propagates the
+; debug location to new `add` and `mul` from the original binary operator
+; they replaced (`%3` in both `@add_reassociate` and `@mul_reassociate`).
+; Because there are more than one identical binary operators in the optimized
+; program, autogenerated checks are used to check the whole program.
+
+define void @add_reassociate(i32 %a, i32 %b, i32 %c) !dbg !5 {
+; CHECK-LABEL: define void @add_reassociate(
+; CHECK-SAME: i32 [[A:%.*]], i32 [[B:%.*]], i32 [[C:%.*]]) !dbg [[DBG5:![0-9]+]] {
+; CHECK-NEXT:    [[TMP1:%.*]] = add i32 [[A]], [[C]], !dbg [[DBG8:![0-9]+]]
+; CHECK-NEXT:    call void @foo(i32 [[TMP1]]), !dbg [[DBG9:![0-9]+]]
+; CHECK-NEXT:    [[TMP2:%.*]] = add i32 [[TMP1]], [[B]], !dbg [[DBG10:![0-9]+]]
+; CHECK-NEXT:    call void @foo(i32 [[TMP2]]), !dbg [[DBG11:![0-9]+]]
+; CHECK-NEXT:    ret void, !dbg [[DBG12:![0-9]+]]
+;
+  %1 = add i32 %a, %c, !dbg !8
+  call void @foo(i32 %1), !dbg !9
+  %2 = add i32 %b, %c, !dbg !10
+  %3 = add i32 %a, %2, !dbg !11
+  call void @foo(i32 %3), !dbg !12
+  ret void, !dbg !13
+}
+
+define void @mul_reassociate(i32 %a, i32 %b, i32 %c) !dbg !14 {
+; CHECK-LABEL: define void @mul_reassociate(
+; CHECK-SAME: i32 [[A:%.*]], i32 [[B:%.*]], i32 [[C:%.*]]) !dbg [[DBG13:![0-9]+]] {
+; CHECK-NEXT:    [[TMP1:%.*]] = mul i32 [[A]], [[C]], !dbg [[DBG14:![0-9]+]]
+; CHECK-NEXT:    call void @foo(i32 [[TMP1]]), !dbg [[DBG15:![0-9]+]]
+; CHECK-NEXT:    [[TMP2:%.*]] = mul i32 [[TMP1]], [[B]], !dbg [[DBG16:![0-9]+]]
+; CHECK-NEXT:    call void @foo(i32 [[TMP2]]), !dbg [[DBG17:![0-9]+]]
+; CHECK-NEXT:    ret void, !dbg [[DBG18:![0-9]+]]
+;
+  %1 = mul i32 %a, %c, !dbg !15
+  call void @foo(i32 %1), !dbg !16
+  %2 = mul i32 %a, %b, !dbg !17
+  %3 = mul i32 %2, %c, !dbg !18
+  call void @foo(i32 %3), !dbg !19
+  ret void, !dbg !20
+}
+
+declare void @foo(i32)
+
+!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: "test.ll", directory: "/")
+!2 = !{i32 12}
+!3 = !{i32 0}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "add_reassociate", linkageName: "add_reassociate", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !DILocation(line: 1, column: 1, scope: !5)
+!9 = !DILocation(line: 2, column: 1, scope: !5)
+!10 = !DILocation(line: 3, column: 1, scope: !5)
+!11 = !DILocation(line: 4, column: 1, scope: !5)
+!12 = !DILocation(line: 5, column: 1, scope: !5)
+!13 = !DILocation(line: 6, column: 1, scope: !5)
+!14 = distinct !DISubprogram(name: "mul_reassociate", linkageName: "mul_reassociate", scope: null, file: !1, line: 7, type: !6, scopeLine: 7, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!15 = !DILocation(line: 7, column: 1, scope: !14)
+!16 = !DILocation(line: 8, column: 1, scope: !14)
+!17 = !DILocation(line: 9, column: 1, scope: !14)
+!18 = !DILocation(line: 10, column: 1, scope: !14)
+!19 = !DILocation(line: 11, column: 1, scope: !14)
+!20 = !DILocation(line: 12, column: 1, scope: !14)
+;.
+; 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: "test.ll", directory: {{.*}})
+; CHECK: [[DBG5]] = distinct !DISubprogram(name: "add_reassociate", linkageName: "add_reassociate", scope: null, file: [[META1]], line: 1, type: [[META6:![0-9]+]], scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: [[META0]])
+; CHECK: [[META6]] = !DISubroutineType(types: [[META7:![0-9]+]])
+; CHECK: [[META7]] = !{}
+; CHECK: [[DBG8]] = !DILocation(line: 1, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG9]] = !DILocation(line: 2, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG10]] = !DILocation(line: 4, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG11]] = !DILocation(line: 5, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG12]] = !DILocation(line: 6, column: 1, scope: [[DBG5]])
+; CHECK: [[DBG13]] = distinct !DISubprogram(name: "mul_reassociate", linkageName: "mul_reassociate", scope: null, file: [[META1]], line: 7, type: [[META6]], scopeLine: 7, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: [[META0]])
+; CHECK: [[DBG14]] = !DILocation(line: 7, column: 1, scope: [[DBG13]])
+; CHECK: [[DBG15]] = !DILocation(line: 8, column: 1, scope: [[DBG13]])
+; CHECK: [[DBG16]] = !DILocation(line: 10, column: 1, scope: [[DBG13]])
+; CHECK: [[DBG17]] = !DILocation(line: 11, column: 1, scope: [[DBG13]])
+; CHECK: [[DBG18]] = !DILocation(line: 12, column: 1, scope: [[DBG13]])
+;.

Apochens · 2024-05-20T08:42:44Z

@OCHyams Could you please help in reviewing this PR?

OCHyams

LGTM with two tiny nits, thanks

OCHyams · 2024-05-20T13:16:36Z

llvm/lib/Transforms/Scalar/NaryReassociate.cpp

    break;
  case Instruction::Mul:
    NewI = BinaryOperator::CreateMul(LHS, RHS, "", I->getIterator());
+    NewI->setDebugLoc(I->getDebugLoc());


style nit: we could sink this out the switch. I'll leave it up to you.

OCHyams · 2024-05-20T13:19:18Z

llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll

+; Because there are more than one identical binary operators in the optimized
+; program, autogenerated checks are used to check the whole program.


Can we remove the !dbg attachments (and metadata definitions which will become unused) from all the instructions that do not contribute to the new debug location? That helps to keep the test targeted and easy to read, and reduces the chance it'll get accidentally incorrectly regenerated in the future.

Would the revision be ok?

OCHyams · 2024-05-20T13:47:34Z

llvm/test/Transforms/NaryReassociate/preserving-debugloc-add-mul.ll

@@ -0,0 +1,65 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=nary-reassociate -S | FileCheck %s
+


Suggested change

; Test that NaryReassociate's tryReassociatedBinaryOp() propagates the

; debug location to new `add` and `mul` from the original binary operator

; they replaced (`%3` in both `@add_reassociate` and `@mul_reassociate`).

Can you please re-add your comment?

Oh, I just forget that the update_test_checks.py would delete the comments.🥲

If the PR is ready, could you please merge it? Thanks!

fix missing debugloc updates

6f9ac84

llvmbot added the llvm:transforms label May 17, 2024

dtcxzyw requested review from jmorse and OCHyams May 20, 2024 08:43

OCHyams added wrong-debug debuginfo labels May 20, 2024

OCHyams approved these changes May 20, 2024

View reviewed changes

nit

28e9bee

OCHyams reviewed May 20, 2024

View reviewed changes

nit

09a6eb2

OCHyams merged commit c2f92a3 into llvm:main May 20, 2024
3 of 4 checks passed

PeddleSpam mentioned this pull request May 20, 2024

[AMDGPU] Fix error in #88512. #92770

Merged

Apochens deleted the #92537_naryreassociate_preserving_debugloc branch May 21, 2024 01:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DebugInfo][NaryReassociate] Fix missing debug location updates #92545

[DebugInfo][NaryReassociate] Fix missing debug location updates #92545

Uh oh!

Apochens commented May 17, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 17, 2024 •

edited

Loading

Uh oh!

Apochens commented May 20, 2024

Uh oh!

OCHyams left a comment

Uh oh!

OCHyams May 20, 2024

Uh oh!

OCHyams May 20, 2024

Uh oh!

Apochens May 20, 2024

Uh oh!

OCHyams May 20, 2024

Uh oh!

Apochens May 20, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

		; Because there are more than one identical binary operators in the optimized
		; program, autogenerated checks are used to check the whole program.

		@@ -0,0 +1,65 @@
		; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
		; RUN: opt < %s -passes=nary-reassociate -S \| FileCheck %s

[DebugInfo][NaryReassociate] Fix missing debug location updates #92545

[DebugInfo][NaryReassociate] Fix missing debug location updates #92545

Uh oh!

Conversation

Apochens commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Apochens commented May 20, 2024

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

OCHyams May 20, 2024

Choose a reason for hiding this comment

Uh oh!

OCHyams May 20, 2024

Choose a reason for hiding this comment

Uh oh!

Apochens May 20, 2024

Choose a reason for hiding this comment

Uh oh!

OCHyams May 20, 2024

Choose a reason for hiding this comment

Uh oh!

Apochens May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Apochens commented May 17, 2024 •

edited

Loading

llvmbot commented May 17, 2024 •

edited

Loading

Apochens May 20, 2024 •

edited

Loading