Don't process debug intrinsics in InstCombine

[dmikulin]

Summary

In debug builds non-trivial amount of time is spent in InstCombine processing @llvm.dbg.* calls in visitCallInst(). They can be safely ignored.
Added visitDbgInfoIntrinsic() to InstCombiner to intercept and skip them.

[davide]

Adding reviewers + llvm-commits.
This patch seems incomplete. Please upload the correct version.

[davide]

Can you add a test?

[dmikulin]

That's the patch, 1 line. This added visitor function will intercept @llvm.declare and @llvm.value calls and ignore them.

I looked into adding a test case. There is no visible trace of processing these nodes other than -debug output, which in instcombine case dump all nodes before they are being processed. So there's still a trace like "Visiting call @llvm.dbg.value" eve though the visitor routine ignores it.

[dmikulin]

We can skip debug intrinsics from adding them to the InstCombine worklist.
With this change I can create a test case. And it shaves a few more seconds off compile time.

[craig.topper]

include/llvm/Transforms/InstCombine/InstCombineWorklist.h

64 ↗

(On Diff #95831)

Should you skip it when the List being passed in here is being populated?

[davide]

See comment. Also now I think you can probably write a test to make sure the instruction is never inserted in the worklist.

include/llvm/Transforms/InstCombine/InstCombineWorklist.h

64 ↗

(On Diff #95831)

I'd hoist the insertion in the working list in a separate helper so that we don't have to copy the check in multiple places.

[dmikulin]

Skip debug intrinsics early.

[davide]

lib/Transforms/InstCombine/InstructionCombining.cpp

3055–3059

Please expand this comment to explain why we want to skip them.

[craig.topper]

lib/Transforms/InstCombine/InstructionCombining.cpp

3055–3059

I'm really sorry to keep asking this to be moved, but is there any reason to run any of the code above here for debug intrinsics?

[dmikulin]

include/llvm/Transforms/InstCombine/InstCombineWorklist.h

64 ↗

(On Diff #95831)

Passing in the initial list from AddReachableCodeToWorklist() without @llvm.dbg works too. I'll make the change.
Do we even need the check in Add() if the initial work list has no @llvm.dbg?

lib/Transforms/InstCombine/InstructionCombining.cpp

3055–3059

// Skip processing debug intrinsics in InstCombine. Processing these call instructions
// consumes non-trivial amount of time and provides no value for the optimization.

Is this OK?

3055–3059

I'm not sure how far up I can move the check. Top of the for loop? But won't we potentially miss DCE and constant folds above?

[dmikulin]

Added a test case to the patch.

[dmikulin]

lib/Transforms/InstCombine/InstructionCombining.cpp

3055–3059

Any thoughts on this?

[dmikulin]

I don't think I saw any more feedback after my last comment. Can someone please reply? Is this change good to go?

[craig.topper]

test/Transforms/InstCombine/debuginfo-skip.ll

3

What is the input to this second run of FileCheck? It's not being piped anything.

[dmikulin]

test/Transforms/InstCombine/debuginfo-skip.ll

3

Sorry, forgot to update the test, it's meant to be:
; RUN: cat %t | FileCheck %s --check-prefix=CHECK-IR

[dmikulin]

Rebased the patch.

[davide]

This is still a quite large testcase and I'm under the impression it can be shrunk a bit.

[davide]

test/Transforms/InstCombine/debuginfo-skip.ll

55–59

For example, I'm pretty sure all these attributes are unneeded.

[dmikulin]

test/Transforms/InstCombine/debuginfo-skip.ll

55–59

This test case was generated from a small source. I wanted to make sure that both @llvm.dbg.decalre and @llvm.dbg.value are present in the IR. I removed the attributes and shortened some strings. Some debug info can probably be removed, but I didn't want to mess with the consistency of the IR to reduce the size.

[davide]

What do you mean with "consistency" of IR?
This test is checking that we skip llvm.dbg.* instructions, and that's the only thing it should test.
Your test still contains a large amount of unneeded stuffs, which makes it very hard to maintain and to look for somebody else in the future.
Among others, lifetime.start/end are largely unneeded.
If there's a good reason to keep all this boilerplate around, please elaborate.

[dmikulin]

Removed unused attributes and debug metadata from the test case.

[dmikulin]

Simplified the test case.

[davide]

The test could probably be further reduced, but I think it's fine now.
The change looked good to me a while ago, and still looks good.
Please wait and check if Craig is happy with this.

[craig.topper]

I'm happy.

[dmikulin]

Changes committed.

lib/Transforms/InstCombine/InstructionCombining.cpp

for (BasicBlock::iterator BBI = BB->begin(), E = BB->end(); BBI != E; ) {
           DEBUG(dbgs() << "IC: ConstFold operand of: " << *Inst
                        << "\n    Old = " << *C
                        << "\n    New = " << *FoldRes << '\n');
           U = FoldRes;
           MadeIRChange = true;
         }
       }
 
+      // Skip processing debug intrinsics in InstCombine. Processing these call instructions
+      // consumes non-trivial amount of time and provides no value for the optimization.
+      if (!isa<DbgInfoIntrinsic>(Inst))
         InstrsForInstCombineWorklist.push_back(Inst);
     }
 
     // Recursively visit successors.  If this is a branch or switch on a
     // constant, only visit the reachable successor.
     TerminatorInst *TI = BB->getTerminator();
     if (BranchInst *BI = dyn_cast<BranchInst>(TI)) {
       if (BI->isConditional() && isa<ConstantInt>(BI->getCondition())) {
         bool CondVal = cast<ConstantInt>(BI->getCondition())->getZExtValue();
         BasicBlock *ReachableBB = BI->getSuccessor(!CondVal);

test/Transforms/InstCombine/debuginfo-skip.ll

+; RUN: opt < %s -instcombine -debug -S -o %t 2>&1 | FileCheck %s
+; RUN: cat %t | FileCheck %s --check-prefix=CHECK-IR
+
+; Debug output from InstCombine should not have any @llvm.dbg.* instructions visited
+; CHECK-NOT: call void @llvm.dbg.
+
+; The resulting IR should still have them
+; CHECK-IR: call void @llvm.dbg.
+
+define i32 @foo(i32 %j) #0 !dbg !7 {
+entry:
+  %j.addr = alloca i32, align 4
+  store i32 %j, i32* %j.addr, align 4
+  call void @llvm.dbg.declare(metadata i32* %j.addr, metadata !11, metadata !12), !dbg !13
+  call void @llvm.dbg.value(metadata i32 10, i64 0, metadata !16, metadata !12), !dbg !15
+  %0 = load i32, i32* %j.addr, align 4, !dbg !14
+  ret i32 %0, !dbg !15
+}
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang 5.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "a.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"PIC Level", i32 2}
+!6 = !{!"clang version 5.0.0 (trunk 302918) (llvm/trunk 302925)"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocalVariable(name: "j", arg: 1, scope: !7, file: !1, line: 2, type: !10)
+!12 = !DIExpression()
+!13 = !DILocation(line: 2, column: 13, scope: !7)
+!14 = !DILocation(line: 5, column: 10, scope: !7)
+!15 = !DILocation(line: 5, column: 3, scope: !7)
+!16 = !DILocalVariable(name: "h", scope: !7, file: !1, line: 4, type: !10)