<div dir="ltr">Thanks Duncan.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 2, 2014 at 1:28 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2014 Aug 19, at 13:55, Akira Hatanaka <<a href="mailto:ahatanak@gmail.com">ahatanak@gmail.com</a>> wrote:<br>
><br>
> On Mon, Aug 18, 2014 at 10:08 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> On Mon, Aug 18, 2014 at 7:09 PM, Akira Hatanaka <<a href="mailto:ahatanaka@apple.com">ahatanaka@apple.com</a>> wrote:<br>
> > Author: ahatanak<br>
> > Date: Mon Aug 18 21:09:57 2014<br>
> > New Revision: 215962<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215962&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215962&view=rev</a><br>
> > Log:<br>
> > [X86, X87 stackifier] Do not mark an operand of a debug instruction as kill.<br>
> ><br>
> > <rdar://problem/16952634><br>
> ><br>
> ><br>
> > Added:<br>
> > llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll<br>
> > Modified:<br>
> > llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp<br>
> ><br>
> > Modified: llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp?rev=215962&r1=215961&r2=215962&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp?rev=215962&r1=215961&r2=215962&view=diff</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp (original)<br>
> > +++ llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp Mon Aug 18 21:09:57 2014<br>
> > @@ -1654,6 +1654,9 @@ void FPS::setKillFlags(MachineBasicBlock<br>
> ><br>
> > for (MachineBasicBlock::reverse_iterator I = MBB.rbegin(), E = MBB.rend();<br>
> > I != E; ++I) {<br>
> > + if (I->isDebugValue())<br>
> > + continue;<br>
> > +<br>
> > BitVector Defs(8);<br>
> > SmallVector<MachineOperand *, 2> Uses;<br>
> > MachineInstr &MI = *I;<br>
> ><br>
> > Added: llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll?rev=215962&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll?rev=215962&view=auto</a><br>
> > ==============================================================================<br>
> > --- llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll (added)<br>
> > +++ llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll Mon Aug 18 21:09:57 2014<br>
> > @@ -0,0 +1,71 @@<br>
> > +; RUN: llc < %s -mcpu=generic -mtriple=i386-apple-darwin -no-integrated-as<br>
><br>
>> Generally we've made a habit of checking in the user-level source<br>
>> along with the IR for debug info tests, to make them easier to<br>
>> maintain in the future, should the schema change greatly, etc. But I<br>
>> realize this is swift code (guessing by the 'sw' prefix in the label<br>
>> names) & with the swift compiler not being part of the LLVM project (&<br>
>> still changing a lot) adding swift code here's probably not much help.<br>
>><br>
>> At least a bit more detailed documentation about which part of all of<br>
>> this code is relevant might be helpful?<br>
>><br>
>> Could the code be simplified (generally we try to simplify the<br>
>> input/source code, rather than hand-simplify the IR, in these debug<br>
>> info tests - but since there's no source, the IR could be<br>
>> hand-simplified (but I'd suggest trying to simplify the input source<br>
>> as much as possible first - since that'll also simplify the metadata,<br>
>> too)). Perhaps there's a C++ reproduction you could use, in which case<br>
>> we wouldn't have to worry about the swiftyness of the code and it<br>
>> might be more obvious?<br>
><br>
><br>
> The source code is actually a c++ program, not a swift code, so I can check it in if that's necessary.<br>
><br>
> I tried simplifying the source code first and created a new test case as you've suggested. Do the attached tests look fine to you? Also, where do the source c++ programs live in the test directory?<br>
<br>
</div></div>I think you can include the C++ source (commented out) in the .ll file<br>
directly.<br>
<div><div class="h5"><br>
><br>
>> Is this test just checking we don't crash? (I don't see any FileCheck,<br>
>> etc) "does not crash" isn't a great thing to test for. There must've<br>
>> been some behavior that was expected of this code other than "does not<br>
>> crash" - perhaps we could/should test for that behavior?<br>
>><br>
><br>
> The patch fixes a bug where a function in X86FloatingPoint.cpp was trying to add a kill flag to a debug instruction, which was causing an assert to fire. The test is just checking whether the pass runs to completion without crashing. I can certainly add FileCheck check strings to the test, but to be honest, I don't know what difference that would make. Do you have any suggestions on what it should check? Would something like "; CHECK: function_label" be sufficient?<br>
><br>
><br>
><br>
> > +<br>
> > +@g1 = global double 0.000000e+00, align 8<br>
> > +@g2 = global i32 0, align 4<br>
> > +<br>
> > +define void @_Z16fpuop_arithmeticjj(i32, i32) {<br>
> > +entry:<br>
> > + switch i32 undef, label %sw.bb.i1921 [<br>
> > + ]<br>
> > +<br>
> > +sw.bb261: ; preds = %entry, %entry<br>
> > + unreachable<br>
> > +<br>
> > +sw.bb.i1921: ; preds = %if.end504<br>
> > + switch i32 undef, label %if.end511 [<br>
> > + i32 1, label %sw.bb27.i<br>
> > + ]<br>
> > +<br>
> > +sw.bb27.i: ; preds = %sw.bb.i1921<br>
> > + %conv.i.i1923 = fpext float undef to x86_fp80<br>
> > + br label %if.end511<br>
> > +<br>
> > +if.end511: ; preds = %sw.bb27.i, %sw.bb13.i<br>
> > + %src.sroa.0.0.src.sroa.0.0.2280 = phi x86_fp80 [ %conv.i.i1923, %sw.bb27.i ], [ undef, %sw.bb.i1921 ]<br>
> > + switch i32 undef, label %sw.bb992 [<br>
> > + i32 3, label %sw.bb735<br>
> > + i32 18, label %if.end41.i2210<br>
> > + ]<br>
> > +<br>
> > +sw.bb735: ; preds = %if.end511<br>
> > + %2 = call x86_fp80 asm sideeffect "frndint", "={st},0,~{dirflag},~{fpsr},~{flags}"(x86_fp80 %src.sroa.0.0.src.sroa.0.0.2280)<br>
> > + unreachable<br>
> > +<br>
> > +if.end41.i2210: ; preds = %if.end511<br>
> > + call void @llvm.dbg.value(metadata !{x86_fp80 %src.sroa.0.0.src.sroa.0.0.2280}, i64 0, metadata !20)<br>
> > + unreachable<br>
> > +<br>
> > +sw.bb992: ; preds = %if.end511<br>
> > + ret void<br>
> > +}<br>
> > +<br>
> > +declare void @llvm.dbg.value(metadata, i64, metadata)<br>
> > +<br>
> > +!<a href="http://llvm.dbg.cu" target="_blank">llvm.dbg.cu</a> = !{!0}<br>
> > +!llvm.module.flags = !{!24, !25}<br>
> > +!0 = metadata !{i32 786449, metadata !1, i32 4, metadata !"clang version 3.6.0 (<a href="http://llvm.org/git/clang" target="_blank">http://llvm.org/git/clang</a> 8444ae7cfeaefae031f8fedf0d1435ca3b14d90b) (<a href="http://llvm.org/git/llvm" target="_blank">http://llvm.org/git/llvm</a> 886f0101a7d176543b831f5efb74c03427244a55)", i1 true, metadata !"", i32 0, metadata !2, metadata !2, metadata !3, metadata !21, metadata !2, metadata !"", i32 1} ; [ DW_TAG_compile_unit ] [x87stackifier/fpu_ieee.cpp] [DW_LANG_C_plus_plus]<br>
> > +!1 = metadata !{metadata !"fpu_ieee.cpp", metadata !"x87stackifier"}<br>
> > +!2 = metadata !{}<br>
> > +!3 = metadata !{metadata !4}<br>
> > +!4 = metadata !{i32 786478, metadata !5, metadata !6, metadata !"fpuop_arithmetic", metadata !"fpuop_arithmetic", metadata !"_Z16fpuop_arithmeticjj", i32 11, metadata !7, i1 false, i1 true, i32 0, i32 0, null, i32 256, i1 true, void (i32, i32)* @_Z16fpuop_arithmeticjj, null, null, metadata !10, i32 13} ; [ DW_TAG_subprogram ] [line 11] [def] [scope 13] [fpuop_arithmetic]<br>
> > +!5 = metadata !{metadata !"f1.cpp", metadata !"x87stackifier"}<br>
> > +!6 = metadata !{i32 786473, metadata !5} ; [ DW_TAG_file_type ] [x87stackifier/f1.cpp]<br>
> > +!7 = metadata !{i32 786453, i32 0, null, metadata !"", i32 0, i64 0, i64 0, i64 0, i32 0, null, metadata !8, i32 0, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]<br>
> > +!8 = metadata !{null, metadata !9, metadata !9}<br>
> > +!9 = metadata !{i32 786468, null, null, metadata !"unsigned int", i32 0, i64 32, i64 32, i64 0, i32 0, i32 7} ; [ DW_TAG_base_type ] [unsigned int] [line 0, size 32, align 32, offset 0, enc DW_ATE_unsigned]<br>
> > +!10 = metadata !{metadata !11, metadata !12, metadata !13, metadata !18, metadata !20}<br>
> > +!11 = metadata !{i32 786689, metadata !4, metadata !"", metadata !6, i32 16777227, metadata !9, i32 0, i32 0} ; [ DW_TAG_arg_variable ] [line 11]<br>
> > +!12 = metadata !{i32 786689, metadata !4, metadata !"", metadata !6, i32 33554443, metadata !9, i32 0, i32 0} ; [ DW_TAG_arg_variable ] [line 11]<br>
> > +!13 = metadata !{i32 786688, metadata !4, metadata !"x", metadata !6, i32 14, metadata !14, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [x] [line 14]<br>
> > +!14 = metadata !{i32 786454, metadata !5, null, metadata !"fpu_extended", i32 3, i64 0, i64 0, i64 0, i32 0, metadata !15} ; [ DW_TAG_typedef ] [fpu_extended] [line 3, size 0, align 0, offset 0] [from fpu_register]<br>
> > +!15 = metadata !{i32 786454, metadata !5, null, metadata !"fpu_register", i32 2, i64 0, i64 0, i64 0, i32 0, metadata !16} ; [ DW_TAG_typedef ] [fpu_register] [line 2, size 0, align 0, offset 0] [from uae_f64]<br>
> > +!16 = metadata !{i32 786454, metadata !5, null, metadata !"uae_f64", i32 1, i64 0, i64 0, i64 0, i32 0, metadata !17} ; [ DW_TAG_typedef ] [uae_f64] [line 1, size 0, align 0, offset 0] [from double]<br>
> > +!17 = metadata !{i32 786468, null, null, metadata !"double", i32 0, i64 64, i64 64, i64 0, i32 0, i32 4} ; [ DW_TAG_base_type ] [double] [line 0, size 64, align 64, offset 0, enc DW_ATE_float]<br>
> > +!18 = metadata !{i32 786688, metadata !4, metadata !"a", metadata !6, i32 15, metadata !19, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [a] [line 15]<br>
> > +!19 = metadata !{i32 786468, null, null, metadata !"int", i32 0, i64 32, i64 32, i64 0, i32 0, i32 5} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]<br>
> > +!20 = metadata !{i32 786688, metadata !4, metadata !"value", metadata !6, i32 16, metadata !14, i32 0, i32 0} ; [ DW_TAG_auto_variable ] [value] [line 16]<br>
> > +!21 = metadata !{metadata !22, metadata !23}<br>
> > +!22 = metadata !{i32 786484, i32 0, null, metadata !"g1", metadata !"g1", metadata !"", metadata !6, i32 5, metadata !14, i32 0, i32 1, double* @g1, null} ; [ DW_TAG_variable ] [g1] [line 5] [def]<br>
> > +!23 = metadata !{i32 786484, i32 0, null, metadata !"g2", metadata !"g2", metadata !"", metadata !6, i32 6, metadata !19, i32 0, i32 1, i32* @g2, null} ; [ DW_TAG_variable ] [g2] [line 6] [def]<br>
> > +!24 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}<br>
> > +!25 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}<br>
> ><br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div>> <fpstack-debuginstr-kill.cpp><fpstack-debuginstr-kill.ll>_______________________________________________<br>
<div class="HOEnZb"><div class="h5">> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div>