<div dir="ltr">(sorry, seems I lost this thread at some point)<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 19, 2014 at 10:55 AM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="h5">On Mon, Aug 18, 2014 at 10:08 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
</div></div><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>On Mon, Aug 18, 2014 at 7:09 PM, Akira Hatanaka <<a href="mailto:ahatanaka@apple.com" target="_blank">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>
</div></div>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></blockquote><div><br></div></div></div><div>The source code is actually a c++ program, not a swift code, so I can check it in if that's necessary.<br></div><div><br></div><div>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? </div>
</div></div></div></blockquote><div><br></div><div>Still seems like an awful lot of source code to tickle this - is it not possible to further simplify it? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Also, where do the source c++ programs live in the test directory?</div></div></div></div></blockquote><div><br></div><div>usually we just quote it in the test case (see other debug info test cases for examples)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="">
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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>
<div><div><br></div></div></blockquote><div><br></div></div><div>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?</div>
</div></div></div></blockquote><div><br></div><div>Not quite sufficient, I don't think.<br><br>The question is about what the expected behavior was that wasn't being tested.<br><br>I would guess there's some test case that demonstrates that these kill flags are correctly applied, yes? (ie: if you remove the code to add the kill flags entirely, some tests will fail) - it probably makes sense to just add a debug intrinsic to one of those test cases to demonstrate that it behaves correctly even in the presence of a debug intrinsic. (this might be able to be done instead of your test case?) You should demonstrate that doing this demonstrates the problem (by locally reverting your patch and watching the test fail) before changing to it.<br>
<br>I'm not sure how much extra debug info you'll need to enable the debug info intrinsic to stay live through your test case (& not cause assertions or failures for being bogus debug info). Myself or Adrian can try to help with that if you have trouble forming the minimal debug info.<br>
<br>(this is different from the original suggestion of including the raw .ll from the frontend and including the source - but when we can get away with handcrafting one or two pieces of debug info to test an IR transformation, etc, that's preferable)<br>
<br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
> +<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" target="_blank">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" target="_blank">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>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>