Fix inconsistency with/without debug information (-g)

[jeroen.dobbelaere]

Summary

This fixes an inconsistency in code generation when compiling with or without debug information (-g).
When debug information is available in an empty block, the original test would fail, resulting in possibly different code.

[JDevlieghere]

Hi Jeroen, any chance you could add a small test to ensure we'd notice if this regresses in the future?

[jeroen.dobbelaere]

In D49467#1166203, @JDevlieghere wrote:

Hi Jeroen, any chance you could add a small test to ensure we'd notice if this regresses in the future?

Yes, I am working on it.

[jeroen.dobbelaere]

• Updated existing (related) testcase to actually test something
• Added new testcase showing the problem

[probinson]

Please rename the new test to simply 'codegenprepare-and-debug.ll` (the convention of a date on the test name is obsolete, we don't do that for new tests anymore).

llvm/test/Other/2018-07-20-codegenprepare-and-debug.ll

3 ↗

(On Diff #156463)

It would be good to be explicit about the X86 dependency:
; REQUIRES: x86-registered-target

4 ↗

(On Diff #156463)

it's should be its here.

[jeroen.dobbelaere]

Updated.

[JDevlieghere]

Looks good to me, but I'll defer to Paul

[probinson]

It seems like the metadata could be reduced further. In particular, do you really need all those different types? The dbg.value needs only !11, I think you should be able to remove !12 through !18 (and references to them).

[jeroen.dobbelaere]

Reduced the debug information.

I am not sure it is minimal now, but when I tried removing more, I started getting 'invalid debug info' messages.

[jeroen.dobbelaere]

Found one extra line of debug info that could be removed.

[JDevlieghere]

Please run the test through llvm-as | llvm-dis, this will fix the numbering of the DI nodes.

[jeroen.dobbelaere]

Reassembled to reset the debug id's

[probinson]

In D49467#1176829, @jeroen.dobbelaere wrote:

Reassembled to reset the debug id's

Oh nice! I wasn't aware of that trick.

The other thing we like in debug-info tests, is to reduce the function attributes as much as possible. I suspect you can eliminate attributes #1 and instead have @foobar use #0. Do that and LGTM.

[jeroen.dobbelaere]

Reduced function attributes.

[jeroen.dobbelaere]

! In D49467#1177034, @probinson wrote:
Oh nice! I wasn't aware of that trick.

You just need to remember to take a copy of the 'comments' in your file and to copy those manually. Otherwise, the test driver is gone (as I noticed the hard way).

[jeroen.dobbelaere]

Jonas, Paul,

could one of you commit this change ?

Thanks !

Jeroen Dobbelaere

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::isMergingEmptyBlockProfitable(BasicBlock *BB,
   // such empty block (BB), ISel will place COPY instructions in BB, not in the
   // predecessor of BB.
   BasicBlock *Pred = BB->getUniquePredecessor();
   if (!Pred ||
       !(isa<SwitchInst>(Pred->getTerminator()) ||
         isa<IndirectBrInst>(Pred->getTerminator())))
     return true;
 
-  if (BB->getTerminator() != BB->getFirstNonPHI())
+  if (BB->getTerminator() != BB->getFirstNonPHIOrDbg())
     return true;
 
   // We use a simple cost heuristic which determine skipping merging is
   // profitable if the cost of skipping merging is less than the cost of
   // merging : Cost(skipping merging) < Cost(merging BB), where the
   // Cost(skipping merging) is Freq(BB) * (Cost(Copy) + Cost(Branch)), and
   // the Cost(merging BB) is Freq(Pred) * Cost(Copy).
   // Assuming Cost(Copy) == Cost(Branch), we could simplify it to :

llvm/trunk/test/Other/2007-04-24-eliminate-mostly-empty-blocks.ll

-;RUN: opt < %s -codegenprepare -disable-output
+;RUN: opt < %s -codegenprepare -S | FileCheck %s
+
+;CHECK: define void @foo()
+;CHECK-NEXT: entry:
+;CHECK-NEXT:  ret void
+
+;CHECK: cond_next475:
+;CHECK-NEXT: br label %cond_next475
+
 
 define void @foo() {
 entry:
   br i1 false, label %cond_next31, label %cond_true
 
 cond_true:    ; preds = %entry
   br i1 false, label %cond_true19, label %cond_next31
 

llvm/trunk/test/Other/codegenprepare-and-debug.ll

+; RUN: opt -codegenprepare -S < %s | FileCheck %s
+; RUN: opt -strip-debug -codegenprepare -S < %s | FileCheck %s
+; REQUIRES: x86-registered-target
+
+; In its current state, CodeGenPrepare should not optimize empty blocks after a switch
+; (See CodeGenPrepare::isMergingEmptyBlockProfitable)
+; This should also be the case when they contain debug information. (sw.bb block)
+; Check this by verifying that the switch labels remain the same
+
+; CHECK: while.cond:
+; CHECK-NEXT:  switch i32 undef, label %sw.default [
+; CHECK-NEXT:    i32 45, label %sw.bb
+; CHECK-NEXT:    i32 104, label %while.cond.lbl_crit_edge
+; CHECK-NEXT:    i32 122, label %while.cond.lbl_crit_edge
+; CHECK-NEXT:    i32 115, label %while.cond134.preheader
+; CHECK-NEXT:    i32 100, label %sw.bb29
+; CHECK-NEXT:    i32 105, label %sw.bb29
+; CHECK-NEXT:  ]
+
+; ModuleID = 'bugpoint-reduced-instructions.bc'
+source_filename = "foobar.c"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #0
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local void @foobar(i32* nocapture %arg0) local_unnamed_addr #0 {
+entry:
+  br label %while.cond
+
+while.cond:                                       ; preds = %entry
+  switch i32 undef, label %sw.default [
+    i32 45, label %sw.bb
+    i32 104, label %while.cond.lbl_crit_edge
+    i32 122, label %while.cond.lbl_crit_edge
+    i32 115, label %while.cond134.preheader
+    i32 100, label %sw.bb29
+    i32 105, label %sw.bb29
+  ]
+
+while.cond.lbl_crit_edge:                         ; preds = %while.cond, %while.cond
+  br label %lbl
+
+while.cond134.preheader:                          ; preds = %while.cond
+  unreachable
+
+sw.bb:                                            ; preds = %while.cond
+  call void @llvm.dbg.value(metadata i32 3, metadata !11, metadata !DIExpression()), !dbg !15
+  br label %lbl
+
+sw.bb29:                                          ; preds = %while.cond, %while.cond
+  unreachable
+
+sw.default:                                       ; preds = %while.cond
+  unreachable
+
+lbl:                                              ; preds = %sw.bb, %while.cond.lbl_crit_edge
+  %var03.1 = phi i32 [ 0, %while.cond.lbl_crit_edge ], [ 3, %sw.bb ]
+  unreachable
+}
+
+attributes #0 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!9, !10}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version XXX ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, globals: !2)
+!1 = !DIFile(filename: "foobar.c", directory: "./")
+!2 = !{}
+!3 = !{!4, !5}
+!4 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!5 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !6, size: 64)
+!6 = !DISubroutineType(types: !7)
+!7 = !{!4, !4, !8}
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!9 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = !{i32 1, !"wchar_size", i32 4}
+!11 = !DILocalVariable(name: "var03", scope: !12, file: !1, line: 3, type: !4)
+!12 = distinct !DISubprogram(name: "foobar", scope: !1, file: !1, line: 2, type: !13, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
+!13 = !DISubroutineType(types: !14)
+!14 = !{null, !5, !8}
+!15 = !DILocation(line: 4, column: 16, scope: !12)