[DebugInfo][LICM] Fix missing debug location updates #91729

Apochens · 2024-05-10T11:22:31Z

LICM-L937 is tested by preserving-debugloc-fmul.ll.

For LICM-L932 (ReciprocalDivisor), I added the debug location update. However, ReciprocalDivisor would be regarded as a loop invariant in the same function after the creation:

auto ReciprocalDivisor = BinaryOperator::CreateFDiv(One, Divisor);      // Created here
ReciprocalDivisor->setFastMathFlags(I.getFastMathFlags());
SafetyInfo->insertInstructionTo(ReciprocalDivisor, I.getParent());
ReciprocalDivisor->insertBefore(&I);

auto Product =
    BinaryOperator::CreateFMul(I.getOperand(0), ReciprocalDivisor);
Product->setFastMathFlags(I.getFastMathFlags());
SafetyInfo->insertInstructionTo(Product, I.getParent());
Product->insertAfter(&I);
I.replaceAllUsesWith(Product);
eraseInstruction(I, *SafetyInfo, MSSAU);

hoist(*ReciprocalDivisor, DT, CurLoop, CFH.getOrCreateHoistedBlock(BB),  // Hoisted here
    SafetyInfo, MSSAU, SE, ORE);

So, it's debug location would be dropped immediately in the function hoist(), and I didn't create a test for it. However, IMO, assigning a proper debug location for a new instruction is reasonable and appropriate, though the assigned debug location would be manipulated afterwards. I'd like to take any suggestion on this special case.

llvmbot · 2024-05-10T11:23:00Z

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

Fix #91716.

LICM-L937 is tested by preserving-debugloc-fmul.ll.

For LICM-L932 (ReciprocalDivisor), I added the debug location update. However, ReciprocalDivisor would be regarded as a loop invariant in the same function after the creation:

auto ReciprocalDivisor = BinaryOperator::CreateFDiv(One, Divisor);      // Created here
ReciprocalDivisor-&gt;setFastMathFlags(I.getFastMathFlags());
SafetyInfo-&gt;insertInstructionTo(ReciprocalDivisor, I.getParent());
ReciprocalDivisor-&gt;insertBefore(&amp;I);

auto Product =
    BinaryOperator::CreateFMul(I.getOperand(0), ReciprocalDivisor);
Product-&gt;setFastMathFlags(I.getFastMathFlags());
SafetyInfo-&gt;insertInstructionTo(Product, I.getParent());
Product-&gt;insertAfter(&amp;I);
I.replaceAllUsesWith(Product);
eraseInstruction(I, *SafetyInfo, MSSAU);

hoist(*ReciprocalDivisor, DT, CurLoop, CFH.getOrCreateHoistedBlock(BB),  // Hoisted here
    SafetyInfo, MSSAU, SE, ORE);

So, it's debug location would be dropped immediately in the function hoist(), and I didn't create a test for it. However, IMO, assigning a proper debug location for a new instruction is reasonable and appropriate, though the assigned debug location would be manipulated afterwards. I'd like to take any suggestion on this special case.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/LICM.cpp (+2)
(added) llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll (+78)

diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index e50413de46b1b..6aa4188d1cc4d 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -933,12 +933,14 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
         ReciprocalDivisor->setFastMathFlags(I.getFastMathFlags());
         SafetyInfo->insertInstructionTo(ReciprocalDivisor, I.getParent());
         ReciprocalDivisor->insertBefore(&I);
+        ReciprocalDivisor->setDebugLoc(I.getDebugLoc());
 
         auto Product =
             BinaryOperator::CreateFMul(I.getOperand(0), ReciprocalDivisor);
         Product->setFastMathFlags(I.getFastMathFlags());
         SafetyInfo->insertInstructionTo(Product, I.getParent());
         Product->insertAfter(&I);
+        Product->setDebugLoc(I.getDebugLoc());
         I.replaceAllUsesWith(Product);
         eraseInstruction(I, *SafetyInfo, MSSAU);
 
diff --git a/llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll b/llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll
new file mode 100644
index 0000000000000..a43eb83d8f3a4
--- /dev/null
+++ b/llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll
@@ -0,0 +1,78 @@
+; RUN: opt -passes=licm -verify-memoryssa -S < %s | FileCheck %s
+
+; Test that LICM's hoistRegin() 
+
+define zeroext i1 @invariant_denom(double %v) !dbg !5 {
+; CHECK:  loop:
+; CHECK:    [[TMP1:%.*]] = fmul fast double {{.*}}, !dbg [[DBG29:![0-9]+]]
+; CHECK:  [[DBG29]] = !DILocation(line: 5,
+;
+entry:
+  br label %loop, !dbg !25
+
+loop:                                             ; preds = %loop, %entry
+  %v3 = phi i32 [ 0, %entry ], [ %v11, %loop ], !dbg !26
+  %v4 = phi i32 [ 0, %entry ], [ %v12, %loop ], !dbg !27
+  %v5 = uitofp i32 %v4 to double, !dbg !28
+  %v6 = fdiv fast double %v5, %v, !dbg !29
+  %v7 = fptoui double %v6 to i64, !dbg !30
+  %v8 = and i64 %v7, 1, !dbg !31
+  %v9 = xor i64 %v8, 1, !dbg !32
+  %v10 = trunc i64 %v9 to i32, !dbg !33
+  %v11 = add i32 %v10, %v3, !dbg !34
+  %v12 = add nuw i32 %v4, 1, !dbg !35
+  %v13 = icmp eq i32 %v12, -1, !dbg !36
+  br i1 %v13, label %end, label %loop, !dbg !37
+
+end:                                              ; preds = %loop
+  %v15 = phi i32 [ %v11, %loop ], !dbg !38
+  %v16 = icmp ne i32 %v15, 0, !dbg !39
+  ret i1 %v16, !dbg !40
+}
+
+
+!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: "preserving-debugloc.ll", directory: "/")
+!2 = !{i32 16}
+!3 = !{i32 13}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "invariant_denom", linkageName: "invariant_denom", scope: null, file: !1, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9, !11, !12, !14, !15, !16, !17, !18, !19, !20, !21, !23, !24}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !1, line: 2, type: !10)
+!10 = !DIBasicType(name: "ty32", size: 32, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !5, file: !1, line: 3, type: !10)
+!12 = !DILocalVariable(name: "3", scope: !5, file: !1, line: 4, type: !13)
+!13 = !DIBasicType(name: "ty64", size: 64, encoding: DW_ATE_unsigned)
+!14 = !DILocalVariable(name: "4", scope: !5, file: !1, line: 5, type: !13)
+!15 = !DILocalVariable(name: "5", scope: !5, file: !1, line: 6, type: !13)
+!16 = !DILocalVariable(name: "6", scope: !5, file: !1, line: 7, type: !13)
+!17 = !DILocalVariable(name: "7", scope: !5, file: !1, line: 8, type: !13)
+!18 = !DILocalVariable(name: "8", scope: !5, file: !1, line: 9, type: !10)
+!19 = !DILocalVariable(name: "9", scope: !5, file: !1, line: 10, type: !10)
+!20 = !DILocalVariable(name: "10", scope: !5, file: !1, line: 11, type: !10)
+!21 = !DILocalVariable(name: "11", scope: !5, file: !1, line: 12, type: !22)
+!22 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+!23 = !DILocalVariable(name: "12", scope: !5, file: !1, line: 14, type: !10)
+!24 = !DILocalVariable(name: "13", scope: !5, file: !1, line: 15, type: !22)
+!25 = !DILocation(line: 1, column: 1, scope: !5)
+!26 = !DILocation(line: 2, column: 1, scope: !5)
+!27 = !DILocation(line: 3, column: 1, scope: !5)
+!28 = !DILocation(line: 4, column: 1, scope: !5)
+!29 = !DILocation(line: 5, column: 1, scope: !5)
+!30 = !DILocation(line: 6, column: 1, scope: !5)
+!31 = !DILocation(line: 7, column: 1, scope: !5)
+!32 = !DILocation(line: 8, column: 1, scope: !5)
+!33 = !DILocation(line: 9, column: 1, scope: !5)
+!34 = !DILocation(line: 10, column: 1, scope: !5)
+!35 = !DILocation(line: 11, column: 1, scope: !5)
+!36 = !DILocation(line: 12, column: 1, scope: !5)
+!37 = !DILocation(line: 13, column: 1, scope: !5)
+!38 = !DILocation(line: 14, column: 1, scope: !5)
+!39 = !DILocation(line: 15, column: 1, scope: !5)
+!40 = !DILocation(line: 16, column: 1, scope: !5)

OCHyams

Thanks for another patch. Code and test SGTM, but there are a couple of questions first.

So, it's debug location would be dropped immediately in the function hoist(), and I didn't create a test for it. However, IMO, assigning a proper debug location for a new instruction is reasonable and appropriate, though the assigned debug location would be manipulated afterwards. I'd like to take any suggestion on this special case.

This is an interesting observation, that hoist calls updateLocationAfterHoist which calls dropLocation.

I think there's a future in which we preserve more location information even after hoists - this is an area we're actively looking into right now (cc @SLTozer).

Given that, I would actually be in favour of doing the location propagation as you have done, and also adding an assert after the hoist which check that the location has been dropped (I think assert(!ReciprocalDivisor.getDebugLoc()) should work? ), with a comment pointing out that if the assert fires a test case should be added and it's safe to remove the assert.

This may end up being an unpopular opinion (propagating the location is a noop because it'll get dropped, but it still has a runtime cost). @pogo59 wdyt? I might be making it up but I think I recall a similar discussion happening recently where you were involved.

OCHyams · 2024-05-10T14:30:28Z

llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll

@@ -0,0 +1,78 @@
+; RUN: opt -passes=licm -verify-memoryssa -S < %s | FileCheck %s
+
+; Test that LICM's hoistRegin() 


Is this comment unfinished?

Oh, that must be a commit mistake. I'll fix it.

OCHyams · 2024-05-10T14:34:41Z

llvm/test/Transforms/LICM/preversing-debugloc-fmul.ll

+!18 = !DILocalVariable(name: "8", scope: !5, file: !1, line: 9, type: !10)
+!19 = !DILocalVariable(name: "9", scope: !5, file: !1, line: 10, type: !10)
+!20 = !DILocalVariable(name: "10", scope: !5, file: !1, line: 11, type: !10)
+!21 = !DILocalVariable(name: "11", scope: !5, file: !1, line: 12, type: !22)


if you give opt the option --debugify-level=locations then the debugify pass only adds !dbg metadata and doesn't insert dbg.values or create DILocalVariables.

In any case, please can you remove these unused variables?

You could also remove remove !dbg attachments and the !DILocations other than the one for line: 5, as that's the only one used in the test, but this isn't super important (feel free to ignore).

pogo59 · 2024-05-10T15:04:20Z

This may end up being an unpopular opinion (propagating the location is a noop because it'll get dropped, but it still has a runtime cost). @pogo59 wdyt? I might be making it up but I think I recall a similar discussion happening recently where you were involved.

I vaguely remember that, but not in enough detail to be sure I will say something consistent here.

It would be an unusual use of assert for sure, and I think it would be preferable to have a test that checks the location is lost. Then the test is already written, it will fail when we handle line locations differently, and a comment in the test can say we expect this to need updating when we fix hoist to do the right thing.

OCHyams · 2024-05-10T15:10:14Z

It would be an unusual use of assert for sure, and I think it would be preferable to have a test that checks the location is lost. Then the test is already written, it will fail when we handle line locations differently, and a comment in the test can say we expect this to need updating when we fix hoist to do the right thing.

+1, that makes a lot more sense.

Apochens · 2024-05-10T15:27:12Z

It would be an unusual use of assert for sure, and I think it would be preferable to have a test that checks the location is lost. Then the test is already written, it will fail when we handle line locations differently, and a comment in the test can say we expect this to need updating when we fix hoist to do the right thing.

Actually, the fdiv with debug location dropped can be tested by the same test case (ie., preserving-debugloc-fmul.ll). I can add checks for that in this test and a comment pointing out this special case. Would that be acceptable?

OCHyams · 2024-05-10T15:32:32Z

Would that be acceptable?

Sounds good, yes please

OCHyams

The comment in the test is very helpful - all LGTM, thanks.

jryans

Looks good to me as well! 😄

I'll go ahead and merge this now.

add debugloc updates for fdiv and fmul

c219efb

llvmbot added the llvm:transforms label May 10, 2024

jryans added the debuginfo label May 10, 2024

jryans requested review from jryans, SLTozer and OCHyams May 10, 2024 14:16

OCHyams reviewed May 10, 2024

View reviewed changes

add checks for dropping the debugloc of fdiv

dc9899d

OCHyams approved these changes May 10, 2024

View reviewed changes

jryans approved these changes May 11, 2024

View reviewed changes

jryans merged commit cdd7821 into llvm:main May 11, 2024
4 checks passed

Apochens deleted the #91716_licm_preserving_debugloc branch May 17, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DebugInfo][LICM] Fix missing debug location updates #91729

[DebugInfo][LICM] Fix missing debug location updates #91729

Uh oh!

Apochens commented May 10, 2024

Uh oh!

llvmbot commented May 10, 2024 •

edited

Loading

Uh oh!

OCHyams left a comment

Uh oh!

OCHyams May 10, 2024

Uh oh!

Apochens May 10, 2024

Uh oh!

OCHyams May 10, 2024

Uh oh!

pogo59 commented May 10, 2024

Uh oh!

OCHyams commented May 10, 2024

Uh oh!

Apochens commented May 10, 2024

Uh oh!

OCHyams commented May 10, 2024

Uh oh!

OCHyams left a comment

Uh oh!

jryans left a comment

Uh oh!

Uh oh!

Uh oh!

		@@ -0,0 +1,78 @@
		; RUN: opt -passes=licm -verify-memoryssa -S < %s \| FileCheck %s

		; Test that LICM's hoistRegin()

[DebugInfo][LICM] Fix missing debug location updates #91729

[DebugInfo][LICM] Fix missing debug location updates #91729

Uh oh!

Conversation

Apochens commented May 10, 2024

Uh oh!

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

Uh oh!

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

OCHyams May 10, 2024

Choose a reason for hiding this comment

Uh oh!

Apochens May 10, 2024

Choose a reason for hiding this comment

Uh oh!

OCHyams May 10, 2024

Choose a reason for hiding this comment

Uh oh!

pogo59 commented May 10, 2024

Uh oh!

OCHyams commented May 10, 2024

Uh oh!

Apochens commented May 10, 2024

Uh oh!

OCHyams commented May 10, 2024

Uh oh!

OCHyams left a comment

Choose a reason for hiding this comment

Uh oh!

jryans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented May 10, 2024 •

edited

Loading