[ADCE] Only remove debug intrinsics if non debug instructions are removed

[uabelho]

Summary

We now limit ADCE to only remove debug intrinsics if it does something else
that would invalidate cached analyses anyway.
As we've seen in
https://github.com/llvm/llvm-project/issues/58285
throwing away cached analysis info when only debug instructions are removed
can lead to different code when debug info is present or not present.

[aeubanks]

(I happened to have touched this area, you'll need to rebase)

llvm/lib/Transforms/Scalar/ADCE.cpp

572

can move this into an else of the previous if statement

llvm/test/Transforms/ADCE/debug-info-intrinsic-2.ll

2

can you use update_test_checks.py?

[uabelho]

Rebase and update according to comments.

[uabelho]

In D145051#4161981, @aeubanks wrote:

(I happened to have touched this area, you'll need to rebase)

Thanks for the suggestions. Done!

[StephenTozer]

llvm/lib/Transforms/Scalar/ADCE.cpp

576–577

[uabelho]

Fix spelling mistake.

[uabelho]

Thanks!

[StephenTozer]

LGTM - I think not deleting debug intrinsics if there would be no other codegen changes is the right call for now as a simplest viable fix for the linked issue; hopefully this doesn't result in intrinsic bloat, but if it does that can be solved separately.

[nikic]

This caused a large compile-time regressions in some cases: http://llvm-compile-time-tracker.com/compare.php?from=57833636816a13ccda53714413c532dc81e3b5ff&to=8aa9ab336889ae2eb8e4188036faeb151379ab7b&stat=instructions:u In particular tramp3d-v4 with debuginfo regressed by 15%.

So I think this is probably not the right way to fix this issue. I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

[uabelho]

In D145051#4166656, @nikic wrote:

This caused a large compile-time regressions in some cases: http://llvm-compile-time-tracker.com/compare.php?from=57833636816a13ccda53714413c532dc81e3b5ff&to=8aa9ab336889ae2eb8e4188036faeb151379ab7b&stat=instructions:u In particular tramp3d-v4 with debuginfo regressed by 15%.

So I think this is probably not the right way to fix this issue. I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Should I revert this for now then or do we leave it in-tree while determining what we do about it?

[nikic]

I'd suggest reverting it.

[uabelho]

Ok, reverted due to the compile-time regressions pointed out by @nikic .

[uabelho]

In D145051#4166656, @nikic wrote:

I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Does everyone agree this is the way forward then? If so I can put up a patch doing that on Monday.

[dblaikie]

In D145051#4166919, @uabelho wrote:

In D145051#4166656, @nikic wrote:

I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Does everyone agree this is the way forward then? If so I can put up a patch doing that on Monday.

Probably worth picking up the discussion on the original bug. I think @aeubanks had speculated about this other path - it does seem more brittle (playing whack-a-mole with preserving analyses - can we say it preserves all analyses despite removing debug intrinsics? And then whack-a-mole in the opposite direction to flag certain analyses as unpreserved if they do get broken by the intrinsic removals?)/might have some other concerns.

[uabelho]

In D145051#4168131, @dblaikie wrote:

In D145051#4166919, @uabelho wrote:

In D145051#4166656, @nikic wrote:

I think what we can do instead is to mark MemorySSA as preserved if only debuginfo intrinsics have been removed. (This is safe, MSSA explicitly never tracks debuginfo intrinsics.)

Does everyone agree this is the way forward then? If so I can put up a patch doing that on Monday.

Probably worth picking up the discussion on the original bug. I think @aeubanks had speculated about this other path - it does seem more brittle (playing whack-a-mole with preserving analyses - can we say it preserves all analyses despite removing debug intrinsics? And then whack-a-mole in the opposite direction to flag certain analyses as unpreserved if they do get broken by the intrinsic removals?)/might have some other concerns.

Sure, let's continue in https://github.com/llvm/llvm-project/issues/58285

[uabelho]

Solved in https://reviews.llvm.org/D145478 instead.

llvm/lib/Transforms/Scalar/ADCE.cpp

for (Instruction &I : instructions(F)) {
               break;
             }
           }
         }
       }
     }
   });
 
+  bool FoundNonDebugInstr = false;
+
   // The inverse of the live set is the dead set.  These are those instructions
   // that have no side effects and do not influence the control flow or return
   // value of the function, and may therefore be deleted safely.
   // NOTE: We reuse the Worklist vector here for memory efficiency.
   for (Instruction &I : llvm::reverse(instructions(F))) {
     // Check if the instruction is alive.
     if (isLive(&I))
       continue;
 
     if (auto *DII = dyn_cast<DbgInfoIntrinsic>(&I)) {
       // Avoid removing a dbg.assign that is linked to instructions because it
       // holds information about an existing store.
       if (auto *DAI = dyn_cast<DbgAssignIntrinsic>(DII))
         if (!at::getAssignmentInsts(DAI).empty())
           continue;
       // Check if the scope of this variable location is alive.
       if (AliveScopes.count(DII->getDebugLoc()->getScope()))
         continue;
 
       // Fallthrough and drop the intrinsic.
+    } else {
+      // Remember that we found some non-debug instructions to delete.
+      FoundNonDebugInstr = true;
     }
 
     // Prepare to delete.
     Worklist.push_back(&I);
     salvageDebugInfo(I);
   }
 
+  Changed.ChangedAnything = Changed.ChangedControlFlow || FoundNonDebugInstr;
+
+  // Only actually remove the dead instructions if we know we will invalidate
+  // cached analysis results. If we only found debug intrinsics to remove and
+  // we then invalidate analyses because of that, later passes may behave
+  // differently which then makes the presence of debug info affect code
+  // generation.
+  if (Changed.ChangedAnything) {
     for (Instruction *&I : Worklist)
       I->dropAllReferences();
 
     for (Instruction *&I : Worklist) {
       ++NumRemoved;
       I->eraseFromParent();
     }
-
-  Changed.ChangedAnything = Changed.ChangedControlFlow || !Worklist.empty();
+  }
 
   return Changed;
 }
 
 // A dead region is the set of dead blocks with a common live post-dominator.
 bool AggressiveDeadCodeElimination::updateDeadRegions() {
   LLVM_DEBUG({
     dbgs() << "final dead terminator blocks: " << '\n';

llvm/test/Transforms/ADCE/debug-info-intrinsic-2.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=adce -S < %s | FileCheck %s
+
+; ADCE can remove the dbg.declare in remove_dbg_declare_and_other as it is also
+; removing a non-debug instruction.
+
+define i16 @remove_dbg_declare_and_other() {
+; CHECK-LABEL: @remove_dbg_declare_and_other(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    ret i16 0
+;
+entry:
+  call void @llvm.dbg.declare(metadata ptr undef, metadata !4, metadata !DIExpression()), !dbg !16
+  %dead = add i8 0, 0
+  ret i16 0
+}
+
+; In dont_remove_only_dbg_declare we must leave the dbg.declare since we're not
+; removing any non-debug instructions.
+
+define i16 @dont_remove_only_dbg_declare() {
+; CHECK-LABEL: @dont_remove_only_dbg_declare(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    call void @llvm.dbg.declare(metadata ptr undef, metadata [[META4:![0-9]+]], metadata !DIExpression()), !dbg [[DBG16:![0-9]+]]
+; CHECK-NEXT:    ret i16 0
+;
+entry:
+  call void @llvm.dbg.declare(metadata ptr undef, metadata !4, metadata !DIExpression()), !dbg !16
+  ret i16 0
+}
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
+
+attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+!llvm.dbg.cu = !{}
+!llvm.module.flags = !{!0, !1, !2, !3}
+
+!0 = !{i32 7, !"Dwarf Version", i32 4}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = !{i32 1, !"wchar_size", i32 1}
+!3 = !{i32 7, !"frame-pointer", i32 2}
+!4 = !DILocalVariable(name: "w", scope: !5, file: !6, line: 18, type: !11)
+!5 = distinct !DILexicalBlock(scope: !7, file: !6, line: 18, column: 8)
+!6 = !DIFile(filename: "foo2.c", directory: "/llvm")
+!7 = distinct !DISubprogram(name: "test1", scope: !6, file: !6, line: 14, type: !8, scopeLine: 14, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !10)
+!8 = !DISubroutineType(types: !9)
+!9 = !{}
+!10 = distinct !DICompileUnit(language: DW_LANG_C99, file: !6, producer: "clang version 16.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, globals: !9, splitDebugInlining: false, nameTableKind: None)
+!11 = !DIDerivedType(tag: DW_TAG_typedef, name: "uint64_t", file: !12, line: 60, baseType: !13)
+!12 = !DIFile(filename: "/include/sys/_stdint.h", directory: "")
+!13 = !DIDerivedType(tag: DW_TAG_typedef, name: "__uint64_t", file: !14, line: 108, baseType: !15)
+!14 = !DIFile(filename: "/include/machine/_default_types.h", directory: "")
+!15 = !DIBasicType(name: "unsigned long long", size: 64, encoding: DW_ATE_unsigned)
+!16 = !DILocation(line: 18, column: 8, scope: !5)

llvm/test/Transforms/ADCE/debug-info-intrinsic.ll

 ;;  return 0;
 ;;}
 ;;
 ;;void calls_empty_function_with_unused_variable_in_unused_subscope(void) {
 ;;  { empty_function_with_unused_variable(); }
 ;;  sink();
 ;;}
 
+; Note: The instruction
+;  %dead = add i8 0, 0
+; in
+;  @variable_in_unused_subscope
+; and
+;  @calls_empty_function_with_unused_variable_in_unused_subscope
+; is just there to let ADCE remove debug intrinsics at all. It only removes
+; debug intrinsics if it also found something else to remove.
 declare void @llvm.dbg.value(metadata, metadata, metadata)
 
 declare void @sink()
 
 ; CHECK-LABEL: define void @variable_in_unused_subscope(
 define void @variable_in_unused_subscope() !dbg !4 {
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   call void @sink
 ; CHECK-NEXT:   ret void
 entry:
   call void @llvm.dbg.value(metadata i32 0, metadata !15, metadata !17), !dbg !18
   call void @sink(), !dbg !19
+  %dead = add i8 0, 0
   ret void, !dbg !20
 }
 
 ; CHECK-LABEL: define void @variable_in_parent_scope(
 define void @variable_in_parent_scope() !dbg !7 {
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   call void @llvm.dbg.value
 ; CHECK-NEXT:   call void @sink
 ; CHECK-NEXT:   ret void
 entry:
   call void @llvm.dbg.value(metadata i32 0, metadata !21, metadata !17), !dbg !22
   call void @sink(), !dbg !23
   ret void, !dbg !25
 }
 
 ; CHECK-LABEL: define void @calls_empty_function_with_unused_variable_in_unused_subscope(
 define void @calls_empty_function_with_unused_variable_in_unused_subscope() !dbg !8 {
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   call void @sink
 ; CHECK-NEXT:   ret void
 entry:
   call void @llvm.dbg.value(metadata i32 0, metadata !26, metadata !17), !dbg !28
   call void @sink(), !dbg !31
+  %dead = add i8 0, 0
   ret void, !dbg !32
 }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!14}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
 !1 = !DIFile(filename: "t.c", directory: "/path/to/test/Transforms/ADCE")

llvm/test/Transforms/MergeFunc/mergefunc-preserve-debug-info.ll

 ; OPTIMIZATION_LEVEL_0: define void @f()
 ; OPTIMIZATION_LEVEL_0-NEXT: entry:
 ; OPTIMIZATION_LEVEL_0-NEXT: %call = call i32 @maxA(i32 3, i32 4), !dbg !{{[0-9]+}}
 ; OPTIMIZATION_LEVEL_0-NEXT: %call1 = call i32 @maxB(i32 1, i32 9), !dbg !{{[0-9]+}}
 ; OPTIMIZATION_LEVEL_0-NEXT: ret void, !dbg !{{[0-9]+}}
 
 ; OPTIMIZATION_LEVEL_2: define void @f()
 ; OPTIMIZATION_LEVEL_2-NEXT: entry:
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 3, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 4, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.declare(metadata ptr undef, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.declare(metadata ptr undef, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 4, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 1, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 9, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.declare(metadata ptr undef, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.declare(metadata ptr undef, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
+; OPTIMIZATION_LEVEL_2-NEXT:  call void @llvm.dbg.value(metadata i32 9, metadata !{{[0-9]+}}, metadata !DIExpression()), !dbg !{{[0-9]+}}
 ; OPTIMIZATION_LEVEL_2-NEXT: ret void, !dbg !{{[0-9]+}}
 
   %call = call i32 @maxA(i32 3, i32 4), !dbg !60
   %call1 = call i32 @maxB(i32 1, i32 9), !dbg !61
   ret void, !dbg !62
 }
 
 !llvm.dbg.cu = !{!0}