This is an archive of the discontinued LLVM Phabricator instance.

Differential D49467

Fix inconsistency with/without debug information (-g)
ClosedPublic

Authored by jeroen.dobbelaere on Jul 18 2018, 2:58 AM.

Details

Reviewers
hfinkel
probinson
JDevlieghere
Commits
rG42243df3b9fe: Fix inconsistency with/without debug information (-g)
rL339129: Fix inconsistency with/without debug information (-g)
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.

Diff Detail

Event Timeline

jeroen.dobbelaere created this revision.Jul 18 2018, 2:58 AM
Herald added subscribers: llvm-commits, JDevlieghere, hiraditya, aprantl. · View Herald TranscriptJul 18 2018, 2:58 AM
JDevlieghere added a comment.Jul 18 2018, 3:24 AM

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

jeroen.dobbelaere added a comment.Jul 19 2018, 9:19 AM
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 this revision to Diff 156463.Jul 20 2018, 3:44 AM
  • Updated existing (related) testcase to actually test something
  • Added new testcase showing the problem
probinson added a comment.Jul 20 2018, 8:14 AM

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 this revision to Diff 156509.Jul 20 2018, 8:56 AM

Updated.

JDevlieghere added a comment.Jul 20 2018, 9:15 AM

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

probinson added a comment.Jul 20 2018, 9:19 AM

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 updated this revision to Diff 156978.Jul 24 2018, 1:42 AM

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 updated this revision to Diff 156979.Jul 24 2018, 1:46 AM

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

vsk added a subscriber: vsk.Jul 25 2018, 10:40 AM
JDevlieghere added a comment.Jul 25 2018, 4:22 PM

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

jeroen.dobbelaere updated this revision to Diff 157500.Jul 26 2018, 8:50 AM

Reassembled to reset the debug id's

probinson accepted this revision.Jul 26 2018, 10:43 AM
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.

This revision is now accepted and ready to land.Jul 26 2018, 10:43 AM
jeroen.dobbelaere updated this revision to Diff 157659.Jul 27 2018, 3:09 AM

Reduced function attributes.

jeroen.dobbelaere added a comment.Jul 27 2018, 3:11 AM

! 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).

JDevlieghere accepted this revision.Jul 29 2018, 10:06 AM
jeroen.dobbelaere added a comment.Aug 7 2018, 4:24 AM

Jonas, Paul,

could one of you commit this change ?

Thanks !

Jeroen Dobbelaere

Closed by commit rL339129: Fix inconsistency with/without debug information (-g) (authored by JDevlieghere). · Explain WhyAug 7 2018, 5:14 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
CodeGen/
2 lines
test/
Other/
10 lines
92 lines

Diff 156978

llvm/lib/CodeGen/CodeGenPrepare.cpp

Context not available.
isa<IndirectBrInst>(Pred->getTerminator()))) isa<IndirectBrInst>(Pred->getTerminator())))
return true; return true;
if (BB->getTerminator() != BB->getFirstNonPHI()) if (BB->getTerminator() != BB->getFirstNonPHIOrDbg())
return true; return true;
// We use a simple cost heuristic which determine skipping merging is // We use a simple cost heuristic which determine skipping merging is
Context not available.

llvm/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() { define void @foo() {
entry: entry:

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

  • This file was added.
; 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 #1 {
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 !46, metadata !DIExpression()), !dbg !47
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 }
attributes #1 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!25, !26}
!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version XXX ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !10, globals: !2)
!1 = !DIFile(filename: "foobar.c", directory: "./")
!2 = !{}
!10 = !{!11, !15}
!11 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!15 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !16, size: 64)
!16 = !DISubroutineType(types: !17)
!17 = !{!11, !11, !18}
!18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
!25 = !{i32 2, !"Debug Info Version", i32 3}
!26 = !{i32 1, !"wchar_size", i32 4}
!27 = distinct !DISubprogram(name: "foobar", scope: !1, file: !1, line: 2, type: !28, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !40)
!28 = !DISubroutineType(types: !29)
!29 = !{null, !15, !18}
!40 = !{}
!46 = !DILocalVariable(name: "var03", scope: !27, file: !1, line: 3, type: !11)
!47 = !DILocation(line: 4, column: 16, scope: !27)