[llvm] r325438 - [DebugInfo][FastISel] Fix dropping dbg.value()
Eric Christopher via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 22 11:28:40 PST 2018
Paging Reid as he and I had this exact conversation recently.
-eric
On Thu, Feb 22, 2018 at 11:27 AM <paul.robinson at sony.com> wrote:
> We might also consider revisiting the tactic of FastISel iterating through
> the block in reverse order. The idea there was to be able to reuse values
> that had to be explicitly materialized into registers. However it's not
> clear to me how much of a win this is, given that in some cases it might
> actually force spilling and reloading those registers. And the reverse
> iteration has caused problems with debug info before.
>
> --paulr
>
>
>
> *From:* llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] *On
> Behalf Of *Vedant Kumar via llvm-commits
> *Sent:* Thursday, February 22, 2018 11:18 AM
> *To:* Vedant Kumar
> *Cc:* llvm-commits
> *Subject:* Re: [llvm] r325438 - [DebugInfo][FastISel] Fix dropping
> dbg.value()
>
>
>
> Ah, I attached the skipUnlessSwiftPR patch by mistake, sorry. This is the
> correct patch for D43427 <https://reviews.llvm.org/D43427>:
>
>
>
>
>
> vedant
>
>
>
> On Feb 22, 2018, at 11:14 AM, Vedant Kumar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>
>
> Hi Sander,
>
>
>
> On Feb 22, 2018, at 9:46 AM, Sander De Smalen <Sander.DeSmalen at arm.com>
> wrote:
>
>
>
> Hi Vedant,
>
> Thanks for looking into this. I see your point about not having different
> code-gen when debug-info is enabled, but can you explain what you mean with
> it altering code-gen when a vreg is instantiated for an operand that has
> multiple uses (with at least one non-debug), specifically in the context of
> FastISel?
>
>
>
> That's a great question. I'm not aware of all the subtleties here, but
> from what I understand even the order in which vregs are assigned may alter
> register allocation, and hence codegen. Consider the sequence:
>
>
>
> ... = add i32 %X, %W
>
> ... = add i32 %Y, %Z
>
> DBG_VAL %X
>
>
>
> Since FastISel runs bottom-up, the vreg (and possibly the physical
> register) for %X, %Y, etc. may change due to handling the DBG_VAL too
> eagerly.
>
>
>
> I'm happy to be wrong about this, though, as that would make life much
> simpler :).
>
>
>
>
>
> I figured that if we're trying to match a dbg.value instruction, it is
> because we want to emit a DBG_VALUE (otherwise why would the dbg.value call
> still be there in the first place?), and so we should expect all operands
> to be instantiated. But that assumption is probably incorrect?
>
>
>
> No, I think this is all correct, the tricky part just seems to be how to
> make this happen without accidentally altering register allocation.
>
>
>
>
>
> Would it be an idea for FastISel to temporarily ignore the dbg.value until
> its operands have been instantiated by other (non-debug) instructions,
> before it will (again) try to create the DBG_VALUE?
>
>
>
> Yes! Adrian suggested this to me as well. I have a WIP prototype, which,
> when combined with the patch from https://reviews.llvm.org/D43427 improves
> an end-to-end test I've got.
>
>
>
> Do you mind running your experiments with the patch? I've tested the patch
> against a stage2 build of clang, but wasn't able to trigger the deferred
> debug value emission on anything except synthetic test inputs. Here are the
> patches:
>
>
>
> <0001-FastISel-Try-lowering-dbg.value-instructions-twice-W.patch>
>
> <0001-test-Introduce-the-skipUnlessSwiftPR-decorator.patch>
>
>
>
>
>
> I'm fine reverting the patch for now though.
>
>
>
> Right, I think that would be best for now -- at least to resolve the
> assertion failure.
>
>
>
> thanks!
>
> vedant
>
>
>
>
> Sander
>
>
> On 21/02/2018, 23:51, "vsk at apple.com on behalf of Vedant Kumar" <
> vsk at apple.com> wrote:
>
> 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
>
>
>
>
>
>
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180222/8f078dc2/attachment.html>
More information about the llvm-commits
mailing list