[llvm] r215431 - [MachineCombiner] Fix for ICE bug 20598
Gerolf Hoflehner
ghoflehner at apple.com
Wed Aug 13 13:49:58 PDT 2014
Absolutely. That was on my mind as the follow-up commit, but I think the commit
order you suggest is better.
Thanks
Gerolf
http://reviews.llvm.org/D4890
On Aug 13, 2014, at 9:15 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 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
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140813/691f1557/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DBGValueRemovalCleanup.patch
Type: application/octet-stream
Size: 3149 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140813/691f1557/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140813/691f1557/attachment-0001.html>
More information about the llvm-commits
mailing list