[llvm] r325438 - [DebugInfo][FastISel] Fix dropping dbg.value()
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 21 15:51:05 PST 2018
I discussed this with Adrian offline, who brought up that the general idea of materializing the debug value if it has > 0 uses can still alter code generation. We'll need some other approach.
@Sander, should we revert this change until we have a solution?
vedant
> On Feb 21, 2018, at 2:56 PM, Vedant Kumar <vsk at apple.com> wrote:
>
> Here's a possible fix:
>
> diff --git a/include/llvm/CodeGen/FastISel.h b/include/llvm/CodeGen/FastISel.h
> index 85bb826dcb8..251d76369fc 100644
> --- a/include/llvm/CodeGen/FastISel.h
> +++ b/include/llvm/CodeGen/FastISel.h
> @@ -262,9 +262,10 @@ public:
> /// to the current block. Return true if selection was successful.
> bool selectOperator(const User *I, unsigned Opcode);
>
> - /// \brief Create a virtual register and arrange for it to be assigned the
> - /// value for the given LLVM value.
> - unsigned getRegForValue(const Value *V);
> + /// \brief Create or look up a virtual register and arrange for it to be
> + /// assigned the value for the given LLVM value. A virtual register may only
> + /// be created if \p AllowMaterialize is true.
> + unsigned getRegForValue(const Value *V, bool AllowMaterialize = true);
>
> /// \brief Look up the value to see if its value is already cached in a
> /// register. It may be defined by instructions across blocks or defined
> diff --git a/lib/CodeGen/SelectionDAG/FastISel.cpp b/lib/CodeGen/SelectionDAG/FastISel.cpp
> index 686fe88a2be..7270aeb25b2 100644
> --- a/lib/CodeGen/SelectionDAG/FastISel.cpp
> +++ b/lib/CodeGen/SelectionDAG/FastISel.cpp
> @@ -192,7 +192,7 @@ bool FastISel::hasTrivialKill(const Value *V) {
> cast<Instruction>(*I->user_begin())->getParent() == I->getParent();
> }
>
> -unsigned FastISel::getRegForValue(const Value *V) {
> +unsigned FastISel::getRegForValue(const Value *V, bool AllowMaterialize) {
> EVT RealVT = TLI.getValueType(DL, V->getType(), /*AllowUnknown=*/true);
> // Don't handle non-simple values in FastISel.
> if (!RealVT.isSimple())
> @@ -216,12 +216,18 @@ unsigned FastISel::getRegForValue(const Value *V) {
> return Reg;
>
> // In bottom-up mode, just create the virtual register which will be used
> - // to hold the value. It will be materialized later.
> + // to hold the value. It will be materialized later if it has uses.
> if (isa<Instruction>(V) &&
> (!isa<AllocaInst>(V) ||
> - !FuncInfo.StaticAllocaMap.count(cast<AllocaInst>(V))))
> + !FuncInfo.StaticAllocaMap.count(cast<AllocaInst>(V))) &&
> + (AllowMaterialize || !V->use_empty()))
> return FuncInfo.InitializeRegForValue(V);
>
> + // If we're not allowed to materialize a value (say, because it's a debug
> + // value), bail out now.
> + if (!AllowMaterialize)
> + return 0;
> +
> SavePoint SaveInsertPt = enterLocalValueArea();
>
> // Materialize the value in a register. Emit any instructions in the
> @@ -1235,7 +1241,7 @@ bool FastISel::selectIntrinsicCall(const IntrinsicInst *II) {
> .addImm(0U)
> .addMetadata(DI->getVariable())
> .addMetadata(DI->getExpression());
> - } else if (unsigned Reg = getRegForValue(V)) {
> + } else if (unsigned Reg = getRegForValue(V, /*AllowMaterialize=*/false)) {
> // FIXME: This does not handle register-indirect values at offset 0.
> bool IsIndirect = false;
> BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II, IsIndirect, Reg,
>
> vedant
>
>> On Feb 21, 2018, at 2:40 PM, Vedant Kumar via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> You can reduce this even further by running `opt -strip -metarenamer -debugify` on the file Mikael attached:
>>
>> <reduced.ll>
>>
>> Stepping back a bit, it looks like calling getRegForValue() here may materialize a Value in a new vreg. Doesn't that alter code generation, and if so, is that actually what we want? (As I understand it it's llvm policy that debug info intrinsics shouldn't affect codegen, but perhaps I'm misreading this and that's not what's happening here.)
>>
>> vedant
>>
>>> On Feb 21, 2018, at 3:53 AM, Mikael Holmén via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>> Hi Sander,
>>>
>>> I stumbled upon a case that hits an assert with this patch:
>>>
>>> llc -O0 -mtriple x86_64-unknown-linux-gnu -mcpu=x86-64 -o - reduced.ll
>>>
>>> gives
>>>
>>> llc: ../lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1600: void llvm::SelectionDAGBuilder::CopyToExportRegsIfNeeded(const llvm::Value *): Assertion `!V->use_empty() && "Unused value assigned virtual registers!"' failed.
>>> #0 0x0000000001e85f04 PrintStackTraceSignalHandler(void*) (build-all/bin/llc+0x1e85f04)
>>> #1 0x0000000001e86676 SignalHandler(int) (build-all/bin/llc+0x1e86676)
>>> #2 0x00007f3824026330 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
>>> #3 0x00007f3822c15c37 gsignal /build/eglibc-ripdx6/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
>>> #4 0x00007f3822c19028 abort /build/eglibc-ripdx6/eglibc-2.19/stdlib/abort.c:91:0
>>> #5 0x00007f3822c0ebf6 __assert_fail_base /build/eglibc-ripdx6/eglibc-2.19/assert/assert.c:92:0
>>> #6 0x00007f3822c0eca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
>>> #7 0x0000000001cb1492 llvm::SelectionDAGBuilder::CopyToExportRegsIfNeeded(llvm::Value const*) (build-all/bin/llc+0x1cb1492)
>>> #8 0x0000000001cb06e8 llvm::SelectionDAGBuilder::visit(llvm::Instruction const&) (build-all/bin/llc+0x1cb06e8)
>>> #9 0x0000000001d4ba0e llvm::SelectionDAGISel::SelectBasicBlock(llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, true>, llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, true>, bool&) (build-all/bin/llc+0x1d4ba0e)
>>> #10 0x0000000001d4a501 llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (build-all/bin/llc+0x1d4a501)
>>> #11 0x0000000001d467e7 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (build-all/bin/llc+0x1d467e7)
>>> #12 0x000000000112c011 (anonymous namespace)::X86DAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) (build-all/bin/llc+0x112c011)
>>> #13 0x00000000015f3659 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (build-all/bin/llc+0x15f3659)
>>> #14 0x00000000018fa2b8 llvm::FPPassManager::runOnFunction(llvm::Function&) (build-all/bin/llc+0x18fa2b8)
>>> #15 0x00000000018fa4f8 llvm::FPPassManager::runOnModule(llvm::Module&) (build-all/bin/llc+0x18fa4f8)
>>> #16 0x00000000018fa9d5 llvm::legacy::PassManagerImpl::run(llvm::Module&) (build-all/bin/llc+0x18fa9d5)
>>> #17 0x00000000006d9305 compileModule(char**, llvm::LLVMContext&) (build-all/bin/llc+0x6d9305)
>>> #18 0x00000000006d6a7b main (build-all/bin/llc+0x6d6a7b)
>>> #19 0x00007f3822c00f45 __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:321:0
>>> #20 0x00000000006d427d _start (build-all/bin/llc+0x6d427d)
>>> Stack dump:
>>> 0. Program arguments: build-all/bin/llc -O0 -mtriple x86_64-unknown-linux-gnu -mcpu=x86-64 -o - reduced.ll
>>> 1. Running pass 'Function Pass Manager' on module 'reduced.ll'.
>>> 2. Running pass 'X86 DAG->DAG Instruction Selection' on function '@func_11'
>>>
>>> Regards,
>>> Mikael
>>>
>>> On 02/17/2018 05:42 PM, Sander de Smalen via llvm-commits wrote:
>>>> Author: s.desmalen
>>>> Date: Sat Feb 17 08:42:54 2018
>>>> New Revision: 325438
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=325438&view=rev
>>>> Log:
>>>> [DebugInfo][FastISel] Fix dropping dbg.value()
>>>> Summary:
>>>> https://llvm.org/PR36263 shows that when compiling at -O0 a dbg.value()
>>>> instruction (that remains from an original dbg.declare()) is dropped
>>>> by FastISel. Since FastISel selects instructions by iterating a basic
>>>> block backwards, it drops the dbg.value if one of its operands is not
>>>> yet instantiated by a previously selected instruction.
>>>> Instead of calling 'lookUpRegForValue()' we can call 'getRegForValue()'
>>>> instead that will insert a placeholder for the operand to be filled in
>>>> when continuing the instruction selection.
>>>> Reviewers: aprantl, dblaikie, probinson
>>>> Reviewed By: aprantl
>>>> Subscribers: llvm-commits, dstenb, JDevlieghere
>>>> Differential Revision: https://reviews.llvm.org/D43386
>>>> Added:
>>>> llvm/trunk/test/CodeGen/Generic/dbg_value_fastisel.ll
>>>> Modified:
>>>> llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
>>>> llvm/trunk/test/DebugInfo/X86/fission-ranges.ll
>>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp?rev=325438&r1=325437&r2=325438&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp (original)
>>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/FastISel.cpp Sat Feb 17 08:42:54 2018
>>>> @@ -1235,7 +1235,7 @@ bool FastISel::selectIntrinsicCall(const
>>>> .addImm(0U)
>>>> .addMetadata(DI->getVariable())
>>>> .addMetadata(DI->getExpression());
>>>> - } else if (unsigned Reg = lookUpRegForValue(V)) {
>>>> + } else if (unsigned Reg = getRegForValue(V)) {
>>>> // FIXME: This does not handle register-indirect values at offset 0.
>>>> bool IsIndirect = false;
>>>> BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II, IsIndirect, Reg,
>>>> Added: llvm/trunk/test/CodeGen/Generic/dbg_value_fastisel.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Generic/dbg_value_fastisel.ll?rev=325438&view=auto
>>>> ==============================================================================
>>>> --- llvm/trunk/test/CodeGen/Generic/dbg_value_fastisel.ll (added)
>>>> +++ llvm/trunk/test/CodeGen/Generic/dbg_value_fastisel.ll Sat Feb 17 08:42:54 2018
>>>> @@ -0,0 +1,54 @@
>>>> +; RUN: llc -O0 -stop-after=livedebugvalues -fast-isel=true < %s | FileCheck %s
>>>> +
>>>> +; CHECK: ![[LOCAL:[0-9]+]] = !DILocalVariable(name: "__vla_expr",
>>>> +; CHECK: DBG_VALUE {{.*}} ![[LOCAL]]
>>>> +
>>>> +; ModuleID = '<stdin>'
>>>> +source_filename = "foo.c"
>>>> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
>>>> +target triple = "x86_64-unknown-linux-gnu"
>>>> +
>>>> +; Function Attrs: noinline nounwind optnone uwtable
>>>> +define void @foo(i32 %n) local_unnamed_addr #0 !dbg !7 {
>>>> +entry:
>>>> + %0 = zext i32 %n to i64, !dbg !11
>>>> + %1 = call i8* @llvm.stacksave(), !dbg !12
>>>> + call void @llvm.dbg.value(metadata i64 %0, metadata !13, metadata !DIExpression()), !dbg !12
>>>> + %vla.i = alloca i32, i64 %0, align 16, !dbg !12
>>>> + call void @llvm.stackrestore(i8* %1), !dbg !12
>>>> + ret void, !dbg !12
>>>> +}
>>>> +
>>>> +; Function Attrs: nounwind
>>>> +declare i8* @llvm.stacksave() #1
>>>> +
>>>> +; Function Attrs: nounwind
>>>> +declare void @llvm.stackrestore(i8*) #1
>>>> +
>>>> +; Function Attrs: nounwind readnone speculatable
>>>> +declare void @llvm.dbg.value(metadata, metadata, metadata) #2
>>>> +
>>>> +attributes #0 = { noinline nounwind optnone uwtable }
>>>> +attributes #1 = { nounwind }
>>>> +attributes #2 = { nounwind readnone speculatable }
>>>> +
>>>> +!llvm.dbg.cu = !{!0}
>>>> +!llvm.module.flags = !{!3, !4, !5}
>>>> +!llvm.ident = !{!6}
>>>> +
>>>> +!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 7.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
>>>> +!1 = !DIFile(filename: "foo.c", directory: "/path/to/build")
>>>> +!2 = !{}
>>>> +!3 = !{i32 2, !"Dwarf Version", i32 4}
>>>> +!4 = !{i32 2, !"Debug Info Version", i32 3}
>>>> +!5 = !{i32 1, !"wchar_size", i32 4}
>>>> +!6 = !{!"clang version 7.0.0"}
>>>> +!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 39, isOptimized: false, unit: !0, variables: !2)
>>>> +!8 = !DISubroutineType(types: !9)
>>>> +!9 = !{!10}
>>>> +!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
>>>> +!11 = !DILocation(line: 2, column: 5, scope: !7)
>>>> +!12 = !DILocation(line: 4, column: 5, scope: !7)
>>>> +!13 = !DILocalVariable(name: "__vla_expr", scope: !14, type: !15, flags: DIFlagArtificial)
>>>> +!14 = distinct !DILexicalBlock(scope: !7, file: !1, line: 32, column: 31)
>>>> +!15 = !DIBasicType(name: "long unsigned int", size: 64, encoding: DW_ATE_unsigned)
>>>> Modified: llvm/trunk/test/DebugInfo/X86/fission-ranges.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/fission-ranges.ll?rev=325438&r1=325437&r2=325438&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/test/DebugInfo/X86/fission-ranges.ll (original)
>>>> +++ llvm/trunk/test/DebugInfo/X86/fission-ranges.ll Sat Feb 17 08:42:54 2018
>>>> @@ -17,6 +17,7 @@
>>>> ; CHECK: DW_AT_location [DW_FORM_sec_offset] ([[B:0x[0-9a-z]*]]
>>>> ; CHECK: DW_AT_location [DW_FORM_sec_offset] ([[D:0x[0-9a-z]*]]
>>>> ; CHECK: DW_AT_ranges [DW_FORM_sec_offset] (0x00000000
>>>> +; CHECK: DW_AT_location [DW_FORM_sec_offset] ([[W:0x[0-9a-z]*]]
>>>> ; CHECK-NOT: .debug_loc contents:
>>>> ; CHECK-NOT: Beginning address offset
>>>> ; CHECK: .debug_loc.dwo contents:
>>>> @@ -25,24 +26,27 @@
>>>> ; if they've changed due to a bugfix, change in register allocation, etc.
>>>> ; CHECK: [[A]]:
>>>> -; CHECK-NEXT: Addr idx 2 (w/ length 169): DW_OP_consts +0, DW_OP_stack_value
>>>> +; CHECK-NEXT: Addr idx 2 (w/ length 188): DW_OP_consts +0, DW_OP_stack_value
>>>> ; CHECK-NEXT: Addr idx 3 (w/ length 25): DW_OP_reg0 RAX
>>>> +; CHECK: [[W]]:
>>>> +; CHECK-NEXT: Addr idx 4 (w/ length 20): DW_OP_reg1 RDX
>>>> +; CHECK-NEXT: Addr idx 5 (w/ length 102): DW_OP_breg7 RSP-56
>>>> ; CHECK: [[E]]:
>>>> -; CHECK-NEXT: Addr idx 4 (w/ length 19): DW_OP_reg0 RAX
>>>> +; CHECK-NEXT: Addr idx 6 (w/ length 24): DW_OP_reg0 RAX
>>>> ; CHECK: [[B]]:
>>>> -; CHECK-NEXT: Addr idx 5 (w/ length 17): DW_OP_reg0 RAX
>>>> +; CHECK-NEXT: Addr idx 7 (w/ length 17): DW_OP_reg0 RAX
>>>> ; CHECK: [[D]]:
>>>> -; CHECK-NEXT: Addr idx 6 (w/ length 17): DW_OP_reg0 RAX
>>>> +; CHECK-NEXT: Addr idx 8 (w/ length 21): DW_OP_reg0 RAX
>>>> ; Make sure we don't produce any relocations in any .dwo section (though in particular, debug_info.dwo)
>>>> ; HDR-NOT: .rela.{{.*}}.dwo
>>>> ; Make sure we have enough stuff in the debug_addr to cover the address indexes
>>>> -; (6 is the last index in debug_loc.dwo, making 7 entries of 8 bytes each, 7 * 8
>>>> -; == 56 base 10 == 38 base 16)
>>>> +; (8 is the last index in debug_loc.dwo, making 9 entries of 8 bytes each, 9 * 8
>>>> +; == 72 base 10 == 48 base 16)
>>>> -; HDR: .debug_addr 00000038
>>>> +; HDR: .debug_addr 00000048
>>>> ; HDR-NOT: .rela.{{.*}}.dwo
>>>> ; From the code:
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> <reduced.ll>_______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list