[llvm] r215431 - [MachineCombiner] Fix for ICE bug 20598
David Blaikie
dblaikie at gmail.com
Wed Aug 13 09:15:26 PDT 2014
I'm not terribly familiar with this code, but what you've written
looks quite like some code in DeadMachineInstructionElim - could the
existing code there be refactored into the utility function in a
cleanup commit, then reused where you need it?
- David
On Tue, Aug 12, 2014 at 6:28 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>
> Removing the DBG Values seems to be the current best choice. I added a
> helper routine (eraseFromParentAndUpdateDBGValue) to do that. Does it look
> OK to you?
>
> Thanks
> Gerolf
>
>
>
>
> Index: include/llvm/CodeGen/MachineInstr.h
> ===================================================================
> --- include/llvm/CodeGen/MachineInstr.h (revision 215144)
> +++ include/llvm/CodeGen/MachineInstr.h (working copy)
> @@ -671,6 +671,12 @@
> /// eraseFromBundle() to erase individual bundled instructions.
> void eraseFromParent();
> + /// Unlink 'this' from the containing basic block and delete it.
> + ///
> + /// For all definitions mark their uses in DBG_VALUE nodes
> + /// as undefined. Otherwise like eraseFromParent().
> + void eraseFromParentAndUpdateDBGValue();
> +
> /// Unlink 'this' form its basic block and delete it.
> ///
> /// If the instruction is part of a bundle, the other instructions in the
> Index: lib/CodeGen/MachineCombiner.cpp
> ===================================================================
> --- lib/CodeGen/MachineCombiner.cpp (revision 215151)
> +++ lib/CodeGen/MachineCombiner.cpp (working copy)
> @@ -380,7 +380,7 @@
> MBB->insert((MachineBasicBlock::iterator) & MI,
> (MachineInstr *)InstrPtr);
> for (auto *InstrPtr : DelInstrs)
> - InstrPtr->eraseFromParent();
> + InstrPtr->eraseFromParentAndUpdateDBGValue();
> Changed = true;
> ++NumInstCombined;
> Index: lib/CodeGen/MachineInstr.cpp
> ===================================================================
> --- lib/CodeGen/MachineInstr.cpp (revision 215144)
> +++ lib/CodeGen/MachineInstr.cpp (working copy)
> @@ -895,6 +895,27 @@
> getParent()->erase(this);
> }
> +void MachineInstr::eraseFromParentAndUpdateDBGValue() {
> + assert(getParent() && "Not embedded in a basic block!");
> + MachineBasicBlock *MBB = getParent();
> + MachineFunction *MF = MBB->getParent();
> + assert(MF && "Not embedded in a function!");
> +
> + MachineInstr *MI = (MachineInstr *)this;
> + MachineRegisterInfo &MRI = MF->getRegInfo();
> +
> + for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
> + const MachineOperand &MO = MI->getOperand(i);
> + if (!MO.isReg() || !MO.isDef())
> + continue;
> + unsigned Reg = MO.getReg();
> + if (!TargetRegisterInfo::isVirtualRegister(Reg))
> + continue;
> + MRI.markUsesInDebugValueAsUndef(Reg);
> + }
> + MI->eraseFromParent();
> +}
> +
> void MachineInstr::eraseFromBundle() {
> assert(getParent() && "Not embedded in a basic block!");
> getParent()->erase_instr(this);
> Index: lib/Target/AArch64/AArch64InstrInfo.cpp
> ===================================================================
> --- lib/Target/AArch64/AArch64InstrInfo.cpp (revision 215431)
> +++ lib/Target/AArch64/AArch64InstrInfo.cpp (working copy)
> @@ -2293,8 +2293,7 @@
> return false;
> // Must only used by the user we combine with.
> - // FIXME: handle the case of DBG uses gracefully
> - if (!MRI.hasOneUse(MI->getOperand(0).getReg()))
> + if (!MRI.hasOneNonDBGUse(MI->getOperand(0).getReg()))
> return false;
> return true;
>
>
> On Aug 12, 2014, at 2:21 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>
> Yes I agree. I had made that comment in the bugzilla report, which is still
> open for that reason ...
>
> Gerolf
>
> On Aug 12, 2014, at 7:06 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Tue, Aug 12, 2014 at 12:54 AM, Gerolf Hoflehner <ghoflehner at apple.com>
> wrote:
>
> Author: ghoflehner
> Date: Tue Aug 12 02:54:12 2014
> New Revision: 215431
>
> URL: http://llvm.org/viewvc/llvm-project?rev=215431&view=rev
> Log:
> [MachineCombiner] Fix for ICE bug 20598
>
> The combiner ignored DBG nodes when checking
> the uses of a virtual register.
>
> It combined a sequence like
> %vreg1 = madd %vreg2, %vreg3,...
> DBG_VALUE (%vreg1 ...)
> %vreg4 = add %vreg1,...
> to
> %vreg4 = madd %vreg2, %vreg3
>
> leaving behind a dangling DBG_VALUE with
> a definition. This triggered an assertion
> in the MachineTraceMetrics.cpp module.
>
>
> I take it your change causes the combiner not to perform the above
> combine when the DBG_VALUE is present?
>
> That's not ideal/desired - there's a fairly strong desire/goal to
> ensure that enabling debug information does not change the resulting
> executable code. (so as to reduce the chance of heisenbugs) I believe
> the desired solution here would be to remove the DBG_VALUE and
> preserve the optimization as it was before your change.
>
>
>
> Added:
>
> llvm/trunk/test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll
> Modified:
> llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp
>
> Modified: llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp?rev=215431&r1=215430&r2=215431&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp (original)
> +++ llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.cpp Tue Aug 12 02:54:12
> 2014
> @@ -2293,7 +2293,8 @@ static bool canCombineWithMUL(MachineBas
> return false;
>
> // Must only used by the user we combine with.
> - if (!MRI.hasOneNonDBGUse(MI->getOperand(0).getReg()))
> + // FIXME: handle the case of DBG uses gracefully
> + if (!MRI.hasOneUse(MI->getOperand(0).getReg()))
> return false;
>
> return true;
>
> Added:
> llvm/trunk/test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll?rev=215431&view=auto
> ==============================================================================
> ---
> llvm/trunk/test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll
> (added)
> +++
> llvm/trunk/test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll
> Tue Aug 12 02:54:12 2014
> @@ -0,0 +1,106 @@
> +; RUN: llc < %s -O2 -mtriple=aarch64-none-linux-gnu
> +
> +; Bug 20598
> +
> +
> +define void @test() #0 {
> +entry:
> + br label %for.body, !dbg !39
> +
> +for.body: ; preds = %for.body,
> %entry
> + %arrayidx5 = getelementptr inbounds i32* null, i64 1, !dbg !43
> + %0 = load i32* null, align 4, !dbg !45, !tbaa !46
> + %s1 = sub nsw i32 0, %0, !dbg !50
> + %n1 = sext i32 %s1 to i64, !dbg !50
> + %arrayidx21 = getelementptr inbounds i32* null, i64 3, !dbg !51
> + %add53 = add nsw i64 %n1, 0, !dbg !52
> + %add55 = add nsw i64 %n1, 0, !dbg !53
> + %mul63 = mul nsw i64 %add53, -20995, !dbg !54
> + tail call void @llvm.dbg.value(metadata !{i64 %mul63}, i64 0, metadata
> !30), !dbg !55
> + %mul65 = mul nsw i64 %add55, -3196, !dbg !56
> + %add67 = add nsw i64 0, %mul65, !dbg !57
> + %add80 = add i64 0, 1024, !dbg !58
> + %add81 = add i64 %add80, %mul63, !dbg !58
> + %add82 = add i64 %add81, 0, !dbg !58
> + %shr83351 = lshr i64 %add82, 11, !dbg !58
> + %conv84 = trunc i64 %shr83351 to i32, !dbg !58
> + store i32 %conv84, i32* %arrayidx21, align 4, !dbg !58, !tbaa !46
> + %add86 = add i64 0, 1024, !dbg !59
> + %add87 = add i64 %add86, 0, !dbg !59
> + %add88 = add i64 %add87, %add67, !dbg !59
> + %shr89352 = lshr i64 %add88, 11, !dbg !59
> + %n2 = trunc i64 %shr89352 to i32, !dbg !59
> + store i32 %n2, i32* %arrayidx5, align 4, !dbg !59, !tbaa !46
> + br label %for.body, !dbg !39
> +}
> +
> +; Function Attrs: nounwind readnone
> +declare void @llvm.dbg.value(metadata, i64, metadata) #1
> +
> +attributes #0 = { nounwind "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"
> "no-infs-fp-math"="false" "no-nans-fp-math"="false"
> "stack-protector-buffer-size"="8" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> +attributes #1 = { nounwind readnone }
> +
> +!llvm.dbg.cu = !{!0}
> +!llvm.module.flags = !{!36, !37}
> +!llvm.ident = !{!38}
> +
> +!0 = metadata !{i32 786449, metadata !1, i32 12, metadata !"clang version
> 3.6.0 ", i1 true, metadata !"", i32 0, metadata !2, metadata !2, metadata
> !3, metadata !2, metadata !2, metadata !"", i32 1} ; [] [] []
> +!1 = metadata !{metadata !"test.c", metadata !""}
> +!2 = metadata !{}
> +!3 = metadata !{metadata !4}
> +!4 = metadata !{i32 786478, metadata !1, metadata !5, metadata !"",
> metadata !"", metadata !"", i32 140, metadata !6, i1 false, i1 true, i32 0,
> i32 0, null, i32 256, i1 true, void ()* @test, null, null, metadata !12, i32
> 141} ; [] [] [def] [scope 141] []
> +!5 = metadata !{i32 786473, metadata !1} ; [] []
> +!6 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0,
> i64 0, i32 0, null, metadata !7, i32 0, null, null, null} ; [] [] [from ]
> +!7 = metadata !{null, metadata !8}
> +!8 = metadata !{i32 786447, null, null, metadata !"", i32 0, i64 64, i64
> 64, i64 0, i32 0, metadata !9} ; [] [] []
> +!9 = metadata !{i32 786454, metadata !10, null, metadata !"", i32 30, i64
> 0, i64 0, i64 0, i32 0, metadata !11} ; [] [] [] [from int]
> +!10 = metadata !{metadata !"", metadata !""}
> +!11 = metadata !{i32 786468, null, null, metadata !"int", i32 0, i64 32,
> i64 32, i64 0, i32 0, i32 5} ; [] [int] []
> +!12 = metadata !{metadata !13, metadata !14, metadata !18, metadata !19,
> metadata !20, metadata !21, metadata !22, metadata !23, metadata !24,
> metadata !25, metadata !26, metadata !27, metadata !28, metadata !29,
> metadata !30, metadata !31, metadata !32, metadata !33, metadata !34,
> metadata !35}
> +!13 = metadata !{i32 786689, metadata !4, metadata !"", metadata !5, i32
> 16777356, metadata !8, i32 0, i32 0} ; [] [data] []
> +!14 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!15 = metadata !{i32 786454, metadata !16, null, metadata !"", i32 183, i64
> 0, i64 0, i64 0, i32 0, metadata !17} ; [] [INT32] [] [from long int]
> +!16 = metadata !{metadata !"", metadata !""}
> +!17 = metadata !{i32 786468, null, null, metadata !"", i32 0, i64 64, i64
> 64, i64 0, i32 0, i32 5} ; [] [long int] []
> +!18 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!19 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!20 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!21 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!22 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!23 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [] [] []
> +!24 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 142, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!25 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 143, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!26 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 143, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!27 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 143, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!28 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 143, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!29 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 144, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!30 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 144, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!31 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 144, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!32 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 144, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!33 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 144, metadata !15, i32 0, i32 0} ; [ ] [] []
> +!34 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 145, metadata !8, i32 0, i32 0} ; [ ] [] []
> +!35 = metadata !{i32 786688, metadata !4, metadata !"", metadata !5, i32
> 146, metadata !11, i32 0, i32 0} ; [ ] [] []
> +!36 = metadata !{i32 2, metadata !"Dwarf Version", i32 4}
> +!37 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}
> +!38 = metadata !{metadata !"clang version 3.6.0 "}
> +!39 = metadata !{i32 154, i32 8, metadata !40, null}
> +!40 = metadata !{i32 786443, metadata !1, metadata !41, i32 154, i32 8, i32
> 2, i32 5} ; [ ] []
> +!41 = metadata !{i32 786443, metadata !1, metadata !42, i32 154, i32 8, i32
> 1, i32 4} ; [ ] []
> +!42 = metadata !{i32 786443, metadata !1, metadata !4, i32 154, i32 3, i32
> 0, i32 0} ; [ ] []
> +!43 = metadata !{i32 157, i32 5, metadata !44, null}
> +!44 = metadata !{i32 786443, metadata !1, metadata !42, i32 154, i32 42,
> i32 0, i32 1} ; [ ] []
> +!45 = metadata !{i32 159, i32 5, metadata !44, null}
> +!46 = metadata !{metadata !47, metadata !47, i64 0}
> +!47 = metadata !{metadata !"int", metadata !48, i64 0}
> +!48 = metadata !{metadata !"omnipotent char", metadata !49, i64 0}
> +!49 = metadata !{metadata !"Simple C/C++ TBAA"}
> +!50 = metadata !{i32 160, i32 5, metadata !44, null}
> +!51 = metadata !{i32 161, i32 5, metadata !44, null}
> +!52 = metadata !{i32 188, i32 5, metadata !44, null}
> +!53 = metadata !{i32 190, i32 5, metadata !44, null}
> +!54 = metadata !{i32 198, i32 5, metadata !44, null}
> +!55 = metadata !{i32 144, i32 13, metadata !4, null}
> +!56 = metadata !{i32 200, i32 5, metadata !44, null}
> +!57 = metadata !{i32 203, i32 5, metadata !44, null}
> +!58 = metadata !{i32 207, i32 5, metadata !44, null}
> +!59 = metadata !{i32 208, i32 5, metadata !44, null}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
More information about the llvm-commits
mailing list