[DebugInfo][Reassociate] Fix missing debug location drop

[Apochens]

Fix #95343 .

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

Fix #95343 .

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/Reassociate.cpp (+1)
• (added) llvm/test/Transforms/Reassociate/dropping_debugloc_the_neg.ll (+51)

diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index f36e21b296bd1..73196e60470e0 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -845,6 +845,7 @@ static Value *NegateValue(Value *V, Instruction *BI,
     }
 
     TheNeg->moveBefore(*InsertPt->getParent(), InsertPt);
+    TheNeg->dropLocation();
     if (TheNeg->getOpcode() == Instruction::Sub) {
       TheNeg->setHasNoUnsignedWrap(false);
       TheNeg->setHasNoSignedWrap(false);
diff --git a/llvm/test/Transforms/Reassociate/dropping_debugloc_the_neg.ll b/llvm/test/Transforms/Reassociate/dropping_debugloc_the_neg.ll
new file mode 100644
index 0000000000000..e3a25627a40e0
--- /dev/null
+++ b/llvm/test/Transforms/Reassociate/dropping_debugloc_the_neg.ll
@@ -0,0 +1,51 @@
+; RUN: opt < %s -passes=reassociate -S | FileCheck %s
+
+define void @fn1(i32 %a, i1 %c, ptr %ptr) !dbg !5 {
+; CHECK-LABEL: define void @fn1(
+; CHECK:       for.cond:
+; CHECK:        [[SUB2:%.*]] = sub i32 0, %d.0{{$}}
+; CHECK:       for.body:
+;
+entry:
+  br label %for.cond, !dbg !8
+
+for.cond:                                         ; preds = %for.body, %entry
+  %d.0 = phi i32 [ 1, %entry ], [ 2, %for.body ], !dbg !9
+  br i1 %c, label %for.end, label %for.body, !dbg !10
+
+for.body:                                         ; preds = %for.cond
+  %sub1 = sub i32 %a, %d.0, !dbg !11
+  %dead1 = add i32 %sub1, 1, !dbg !12
+  %dead2 = mul i32 %dead1, 3, !dbg !13
+  %dead3 = mul i32 %dead2, %sub1, !dbg !14
+  %sub2 = sub nsw i32 0, %d.0, !dbg !15
+  store i32 %sub2, ptr %ptr, align 4, !dbg !16
+  br label %for.cond, !dbg !17
+
+for.end:                                          ; preds = %for.cond
+  ret void, !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: "debugloc.ll", directory: "/")
+!2 = !{i32 11}
+!3 = !{i32 0}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = distinct !DISubprogram(name: "fn1", linkageName: "fn1", 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 = !DILocation(line: 7, column: 1, scope: !5)
+!15 = !DILocation(line: 8, column: 1, scope: !5)
+!16 = !DILocation(line: 9, column: 1, scope: !5)
+!17 = !DILocation(line: 10, column: 1, scope: !5)
+!18 = !DILocation(line: 11, column: 1, scope: !5)

[OCHyams]

LGTM - without this change the debug info makes it appear as if the loop body is unconditionally executed (or at least as though it's always stepped into once), dropping the location is definitely the right thing to do.

[OCHyams]

One thought - it might be ok to keep the location if it's not being moved out of the block?

[Apochens]

Yah, you're right. Actually there are test cases in which TheNeg is moved inside its parent block. 10 test cases failed by inserting assert(false) right before moveBefore(), and 2 of them move TheNeg out of the loop's body (causing this misleading debug location) and 8 of them move it just inside one basic block.

If we can decide whether InsertPt is dominated or post-dominated by TheNeg, like using (Post)DominatorTreeAnalysis, then we could choose to drop the location or not. That's my thought, but adding the (Post) Dominator Analysis here requires a lot modification to the pass.

[OCHyams]

I don't think we want to add any analyses here. I think if we just check if the InsertPt BB is different to TheNeg BB (then drop the location) that should be enough?

[Apochens]

I think that is a solution. I'll do that. 🤔

[OCHyams]

LGTM, thanks @Apochens