HomePhabricator

[DebugInfo] Remove some users of DBG_VALUEs IsIndirect field

Authored by jmorse on Oct 15 2019, 3:46 AM.

Description

[DebugInfo] Remove some users of DBG_VALUEs IsIndirect field

This patch kills off a significant user of the "IsIndirect" field of
DBG_VALUE machine insts. Brought up in in PR41675, IsIndirect is
techncally redundant as it can be expressed by the DIExpression of a
DBG_VALUE inst, and it isn't helpful to have two ways of expressing
things.

Rather than setting IsIndirect, have DBG_VALUE creators add an extra deref
to the insts DIExpression. There should now be no appearences of
IsIndirect=True from isel down to LiveDebugVariables / VirtRegRewriter,
which is ensured by an assertion in LDVImpl::handleDebugValue. This means
we also get to delete the IsIndirect handling in LiveDebugVariables. Tests
can be upgraded by for example swapping the following IsIndirect=True
DBG_VALUE:

DBG_VALUE $somereg, 0, !123, !DIExpression(DW_OP_foo)

With one where the indirection is in the DIExpression, by _appending_
a deref:

DBG_VALUE $somereg, $noreg, !123, !DIExpression(DW_OP_foo, DW_OP_deref)

Which both mean the same thing.

Most of the test changes in this patch are updates of that form; also some
changes in how the textual assembly printer handles these insts.

Differential Revision: https://reviews.llvm.org/D68945

llvm-svn: 374877

Event Timeline

cdevadas added inline comments.
/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5522
jmorse added inline comments.Apr 27 2021, 5:17 AM
/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5522

Hmn, that's true -- the revert records there were some conflicts, it's possible that this was a mistake.

As far as I'm aware though the assertion should hold -- that form of variable location (dbg.declare) should only refer to a memory address, and (as far as I'm aware) any memory address shouldn't be split between registers, which is what this code path handles. Do you have a scenario where this assertion fires?

Instead of putting the comment here, I think I should have made the first comment in its actual Phab review D68945.

/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5522

The hidden sret parameter gets split into two register COPY for AMDGPU. All registers are 32-bit in size.

See the test case here:
https://godbolt.org/z/q947cs76h

It works for x86 that supports 64-bit register copy/load and there is no need for register split. May be true for many architectures.

arsenm added a subscriber: arsenm.Apr 27 2021, 7:35 AM
arsenm added inline comments.
/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5522

A single 64-bit argument may be split into multiple components. Those could even be split between a register and a stack slot

jmorse added inline comments.Apr 28 2021, 8:53 AM
/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5522

Lovely :(, sounds like the assertion needs to go then,

Would you be able to upload a patch with a regression test; I've zero familiarity with AMDGPU.

Here is the AMDGPU test that currently asserts. It should fail with the upstream compiler.
Run with: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx908 test.ll -o test.s


target triple = "amdgcn-amd-amdhsa"

%struct.A = type { [100 x i32] }

define dso_local void @_Z4funcv(%struct.A* noalias sret(%struct.A) align 4 %0) #0 !dbg !9 {

call void @llvm.dbg.declare(metadata %struct.A* %0, metadata !20, metadata !DIExpression()), !dbg !21
%2 = getelementptr inbounds %struct.A, %struct.A* %0, i32 0, i32 0, !dbg !22
%3 = getelementptr inbounds [100 x i32], [100 x i32]* %2, i64 0, i64 99, !dbg !23
store i32 1, i32* %3, align 4, !dbg !24
ret void, !dbg !25

}

declare void @llvm.dbg.declare(metadata, metadata, metadata) #0

attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!3, !4, !5, !6, !7}
!llvm.ident = !{!8}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 13.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: fal\
se, nameTableKind: None)
!1 = !DIFile(filename: "example.cpp", directory: "temp")
!2 = !{}
!3 = !{i32 7, !"Dwarf Version", i32 4}
!4 = !{i32 2, !"Debug Info Version", i32 3}
!5 = !{i32 1, !"wchar_size", i32 4}
!6 = !{i32 7, !"uwtable", i32 1}
!7 = !{i32 7, !"frame-pointer", i32 2}
!8 = !{!"clang version 13.0.0"}
!9 = distinct !DISubprogram(name: "func", linkageName: "_Z4funcv", scope: !10, file: !10, line: 5, type: !11, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes:\
!2)
!10 = !DIFile(filename: "example.cpp", directory: "/app")
!11 = !DISubroutineType(types: !12)
!12 = !{!13}
!13 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "A", file: !10, line: 1, size: 3200, flags: DIFlagTypePassByValue, elements: !14, identifier: "_ZTS1A")
!14 = !{!15}
!15 = !DIDerivedType(tag: DW_TAG_member, name: "x", scope: !13, file: !10, line: 2, baseType: !16, size: 3200)
!16 = !DICompositeType(tag: DW_TAG_array_type, baseType: !17, size: 3200, elements: !18)
!17 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!18 = !{!19}
!19 = !DISubrange(count: 100)
!20 = !DILocalVariable(name: "a", scope: !9, file: !10, line: 6, type: !13)
!21 = !DILocation(line: 6, column: 7, scope: !9)
!22 = !DILocation(line: 7, column: 7, scope: !9)
!23 = !DILocation(line: 7, column: 5, scope: !9)
!24 = !DILocation(line: 7, column: 13, scope: !9)
!25 = !DILocation(line: 8, column: 5, scope: !9)