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