[DebugInfo] LLVM ERROR: Broken function found, while removing Debug Intrinsics

[CarlosAlbertoEnciso]

Summary

This patch includes a fix for a compilation failure, introduced by:

https://reviews.llvm.org/D53287
https://reviews.llvm.org/rL345250

The test case that causes the compilation failure, was supplied by Mikael Holmen.

For the given test case, the verifier complains with the error message:

LLVM ERROR: Broken function found, compilation aborted!

The issue has been reported as:

https://bugs.llvm.org/show_bug.cgi?id=40523

[uabelho]

The fix makes sense to me but please let others have an opinion too.

Thanks!

test/CodeGen/X86/bbi-23595.ll

11 ↗

(On Diff #184266)

Perhaps add some positive CHECKs instead that verifies that we get the optimized output that we want?
We only get a single bb with the phi and ret left I think?

[aprantl]

LGTM with a positive CHECK.

[CarlosAlbertoEnciso]

Thanks very much for your invaluable feedback and reviews.

Updated patch to include positive CHECKs on the optimized output.

[CarlosAlbertoEnciso]

test/CodeGen/X86/bbi-23595.ll

11 ↗

(On Diff #184266)

Uploaded a patch to include positive CHECKs.

[dstenb]

Maybe it can be good to rename the test file to either the PR number, or something that says what it verifies? The "bbi-" number refer to one of Ericsson's internal issue trackers.

(Although, I guess that we wouldn't mind if the name was kept as-is :-))

[dstenb]

test/CodeGen/X86/bbi-23595.ll

14 ↗

(On Diff #184473)

It seems to me that the negative check is unnecessary now. The positive checks verify that the llvm.dbg.label is dropped. Also, if the verifier errors out, opt exits with a non-zero exit code, so the test would fail due to that.

[CarlosAlbertoEnciso]

test/CodeGen/X86/bbi-23595.ll

14 ↗

(On Diff #184473)

You are correct. The negative check is not required. I tried the test case without the fix and the it fails.

[CarlosAlbertoEnciso]

Updated patch to remove the negative CHECK. Thanks for your feedback.

[aprantl]

test/CodeGen/X86/bbi-23595.ll

12 ↗

(On Diff #184487)

I would put all this meta-info in the commit message and instead focus on describing what is being tested here. Something like: We're expecting the dbg.label to disappear because ...

[CarlosAlbertoEnciso]

Updated patch to address comments from @aprantl .

Thanks for the feedback.

[CarlosAlbertoEnciso]

test/CodeGen/X86/bbi-23595.ll

12 ↗

(On Diff #184487)

I have updated the patch following your comments and I will use the standard short message in the commit.

I will include in the bugzilla, the following text:

Note: This patch is a complement to pr38762.

For the given test case, the verifier complains with the error message:

LLVM ERROR: Broken function found, compilation aborted!
The issue was introduced at https://reviews.llvm.org/rL345250.

[CarlosAlbertoEnciso]

Thanks @aprantl, @uabelho for your valuable review.

[dstenb]

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

[CarlosAlbertoEnciso]

In D57444#1390350, @dstenb wrote:

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

As this review is closed and committed, I will create another revision just to move the test to the 'Transforms' directory. Thanks

[aprantl]

In D57444#1390579, @CarlosAlbertoEnciso wrote:

In D57444#1390350, @dstenb wrote:

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

As this review is closed and committed, I will create another revision just to move the test to the 'Transforms' directory. Thanks

Don't bother with a review, just do it.

[CarlosAlbertoEnciso]

@aprantl, as per your feedback I have moved the test case to the correct location.

llvm/trunk/include/llvm/Transforms/Utils/Local.h

 
 /// Remove the debug intrinsic instructions for the given instruction.
 void dropDebugUsers(Instruction &I);
 
 /// Hoist all of the instructions in the \p IfBlock to the dominant block
 /// \p DomBlock, by moving its instructions to the insertion point \p InsertPt.
 ///
 /// The moved instructions receive the insertion point debug location values
-/// (DILocations) and their debug intrinsic instructions (dbg.values) are
-/// removed.
+/// (DILocations) and their debug intrinsic instructions are removed.
 void hoistAllInstructionsInto(BasicBlock *DomBlock, Instruction *InsertPt,
                               BasicBlock *BB);
 
 //===----------------------------------------------------------------------===//
 //  Intrinsic pattern matching
 //
 
 /// Try to match a bswap or bitreverse idiom.

llvm/trunk/lib/Transforms/Utils/Local.cpp

void llvm::dropDebugUsers(Instruction &I) {
   for (auto *DII : DbgUsers)
     DII->eraseFromParent();
 }
 
 void llvm::hoistAllInstructionsInto(BasicBlock *DomBlock, Instruction *InsertPt,
                                     BasicBlock *BB) {
   // Since we are moving the instructions out of its basic block, we do not
   // retain their original debug locations (DILocations) and debug intrinsic
-  // instructions (dbg.values).
+  // instructions.
   //
   // Doing so would degrade the debugging experience and adversely affect the
   // accuracy of profiling information.
   //
   // Currently, when hoisting the instructions, we take the following actions:
-  // - Remove their dbg.values.
+  // - Remove their debug intrinsic instructions.
   // - Set their debug locations to the values from the insertion point.
   //
   // As per PR39141 (comment #8), the more fundamental reason why the dbg.values
   // need to be deleted, is because there will not be any instructions with a
   // DILocation in either branch left after performing the transformation. We
   // can only insert a dbg.value after the two branches are joined again.
   //
   // See PR38762, PR39243 for more details.
   //
   // TODO: Extend llvm.dbg.value to take more than one SSA Value (PR39141) to
   // encode predicated DIExpressions that yield different results on different
   // code paths.
   for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
     Instruction *I = &*II;
     I->dropUnknownNonDebugMetadata();
     if (I->isUsedByMetadata())
       dropDebugUsers(*I);
-    if (isa<DbgVariableIntrinsic>(I)) {
+    if (isa<DbgInfoIntrinsic>(I)) {
       // Remove DbgInfo Intrinsics.
       II = I->eraseFromParent();
       continue;
     }
     I->setDebugLoc(InsertPt->getDebugLoc());
     ++II;
   }
   DomBlock->getInstList().splice(InsertPt->getIterator(), BB->getInstList(),

llvm/trunk/test/CodeGen/X86/bbi-23595.ll

+; RUN: opt < %s -S -simplifycfg | FileCheck %s
+
+; In 'simplifycfg', during the flattening of a 'br', the instructions for the
+; 'true' and 'false' parts, are moved out from their respective basic blocks.
+; Their original debug locations (DILocations) and debug intrinsic instructions
+; (dbg.values) are removed.
+; As those basic blocks are now empty, their associated labels are removed.
+;
+; For the given test case, the labels 'W' and 'cleanup4' are removed.
+; We're expecting the dbg.label associated with 'W' to disappear, because
+; the 'W' label was removed.
+
+; CHECK-LABEL: _Z7test_itv()
+; CHECK:       entry:
+; CHECK-NEXT:    %retval.0 = select i1 undef, i16 1, i16 0
+; CHECK-NEXT:    ret i16 0
+
+define i16 @_Z7test_itv() {
+entry:
+  br label %sw.bb
+
+sw.bb:                                            ; preds = %entry
+  br i1 undef, label %W, label %cleanup4
+
+W:                                                ; preds = %sw.bb
+  call void @llvm.dbg.label(metadata !1), !dbg !8
+  br label %cleanup4
+
+cleanup4:                                         ; preds = %W, %sw.bb
+  %retval.0 = phi i16 [ 1, %W ], [ 0, %sw.bb ]
+  ret i16 0
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.label(metadata) #0
+
+attributes #0 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{}
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = !DILabel(scope: !2, name: "W", file: !3, line: 47)
+!2 = distinct !DILexicalBlock(scope: !4, file: !3, line: 40, column: 3)
+!3 = !DIFile(filename: "foo.c", directory: "./")
+!4 = distinct !DISubprogram(name: "test_it", scope: !3, file: !3, line: 35, type: !5, scopeLine: 36, unit: !7)
+!5 = !DISubroutineType(types: !6)
+!6 = !{}
+!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!8 = !DILocation(line: 47, column: 2, scope: !2)