[debuginfo][CorrelatedValuePropagation] Missing debug location after replacement in processSRem function

[yuanboli233]

Summary

The variable Res is updated, however, the debug location is dropped, According to the LLVM debug location update guideline (https://llvm.org/docs/HowToUpdateDebugInfo.html), it seems that the debug location should be kept.

Here is the optimization result for the test case.

Before:

call void @llvm.assume(i1 %c1), !dbg !16
%rem = srem i8 %x, %y, !dbg !17
call void @llvm.dbg.value(metadata i8 %rem, metadata !12, metadata !DIExpression()), !dbg !17

After

call void @llvm.assume(i1 %c1), !dbg !16
%x.nonneg = sub i8 0, %x, !dbg !17
%y.nonneg = sub i8 0, %y, !dbg !17
%rem1 = urem i8 %x.nonneg, %y.nonneg, !dbg !17
%rem1.neg = sub i8 0, %rem1
call void @llvm.dbg.value(metadata i8 %rem1.neg, metadata !12, metadata !DIExpression()), !dbg !17

It seems that there is no reason to drop the debug location for %rem1.neg

[StephenTozer]

Change LGTM, one minor nitpick with the test. Also as a general rule, as per the LLVM Phabricator review guide you should produce the diff with full context (i.e. add -U999999 to the git command).

llvm/test/Transforms/CorrelatedValuePropagation/srem_missing_debugloc.ll

3

There's nothing incorrect with the COM directive here, but it's generally only needed if the commented line actually contains a directive.

30–31

Unless these attributes are necessary for the test, they can be deleted from here and the functions that use them.

[yuanboli233]

Thanks for the feedback! I was busy over the past few days. I will update the revision on the weekend.

[yuanboli233]

updated the revision based on the previous comments.

[jmorse]

LGTM too

[yuanboli233]

Thanks for the feedback! Can you help me to commit the changes as well？

[StephenTozer]

In D132218#3763486, @yuanboli233 wrote:

Thanks for the feedback! Can you help me to commit the changes as well？

Will take care of this, thanks for the patch!

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp

for (Operand &Op : Ops) {
     BO->setDebugLoc(SDI->getDebugLoc());
     Op.V = BO;
   }
 
   auto *URem =
       BinaryOperator::CreateURem(Ops[0].V, Ops[1].V, SDI->getName(), SDI);
   URem->setDebugLoc(SDI->getDebugLoc());
 
-  Value *Res = URem;
+  auto *Res = URem;
 
   // If the divident was non-positive, we need to negate the result.
-  if (Ops[0].D == Domain::NonPositive)
+  if (Ops[0].D == Domain::NonPositive) {
     Res = BinaryOperator::CreateNeg(Res, Res->getName() + ".neg", SDI);
+    Res->setDebugLoc(SDI->getDebugLoc());
+  }
 
   SDI->replaceAllUsesWith(Res);
   SDI->eraseFromParent();
 
   // Try to simplify our new urem.
   processUDivOrURem(URem, LVI);
 
   return true;

llvm/test/Transforms/CorrelatedValuePropagation/srem_missing_debugloc.ll

+; RUN: opt -passes=correlated-propagation -S < %s | FileCheck %s
+; CHECK: %{{[a-zA-Z0-9_]*}} = urem i8 %x.nonneg, %y.nonneg, !dbg ![[DBGLOC:[0-9]+]]
+; CHECK-NEXT: %{{[a-zA-Z0-9_]*}}.neg = sub i8 0, %rem1, !dbg ![[DBGLOC]]
+
+; ModuleID = 'reduced.ll'
+source_filename = "orig.ll"
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "thumbv7m-arm-none-eabi"
+
+; Function Attrs: inaccessiblememonly nocallback nofree nosync nounwind willreturn
+declare void @llvm.assume(i1 noundef) #0
+
+define void @test8_neg_neg(i8 %x, i8 %y) !dbg !5 {
+  %c0 = icmp sle i8 %x, 0, !dbg !13
+  call void @llvm.dbg.value(metadata i1 %c0, metadata !9, metadata !DIExpression()), !dbg !13
+  call void @llvm.assume(i1 %c0), !dbg !14
+  %c1 = icmp sle i8 %y, 0, !dbg !15
+  call void @llvm.dbg.value(metadata i1 %c1, metadata !11, metadata !DIExpression()), !dbg !15
+  call void @llvm.assume(i1 %c1), !dbg !16
+  %rem = srem i8 %x, %y, !dbg !17
+  call void @llvm.dbg.value(metadata i8 %rem, metadata !12, metadata !DIExpression()), !dbg !17
+  ret void, !dbg !18
+}
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+!llvm.debugify = !{!3, !4}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C, file: !2, producer: "debugify", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!2 = !DIFile(filename: "reduced.ll", directory: "/")
+!3 = !{i32 6}
+!4 = !{i32 3}
+!5 = distinct !DISubprogram(name: "test8_neg_neg", linkageName: "test8_neg_neg", scope: null, file: !2, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !8)
+!6 = !DISubroutineType(types: !7)
+!7 = !{}
+!8 = !{!9, !11, !12}
+!9 = !DILocalVariable(name: "1", scope: !5, file: !2, line: 1, type: !10)
+!10 = !DIBasicType(name: "ty8", size: 8, encoding: DW_ATE_unsigned)
+!11 = !DILocalVariable(name: "2", scope: !5, file: !2, line: 3, type: !10)
+!12 = !DILocalVariable(name: "3", scope: !5, file: !2, line: 5, type: !10)
+!13 = !DILocation(line: 1, column: 1, scope: !5)
+!14 = !DILocation(line: 2, column: 1, scope: !5)
+!15 = !DILocation(line: 3, column: 1, scope: !5)
+!16 = !DILocation(line: 4, column: 1, scope: !5)
+!17 = !DILocation(line: 5, column: 1, scope: !5)
+!18 = !DILocation(line: 6, column: 1, scope: !5)