[llvm] r215962 - [X86, X87 stackifier] Do not mark an operand of a debug instruction as kill.
Akira Hatanaka
ahatanak at gmail.com
Wed Sep 3 13:21:02 PDT 2014
On Tue, Sep 2, 2014 at 1:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
> (sorry, seems I lost this thread at some point)
>
>
> On Tue, Aug 19, 2014 at 10:55 AM, Akira Hatanaka <ahatanak at gmail.com>
> wrote:
>
>> On Mon, Aug 18, 2014 at 10:08 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> On Mon, Aug 18, 2014 at 7:09 PM, Akira Hatanaka <ahatanaka at apple.com>
>>> wrote:
>>> > Author: ahatanak
>>> > Date: Mon Aug 18 21:09:57 2014
>>> > New Revision: 215962
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=215962&view=rev
>>> > Log:
>>> > [X86, X87 stackifier] Do not mark an operand of a debug instruction as
>>> kill.
>>> >
>>> > <rdar://problem/16952634>
>>> >
>>> >
>>> > Added:
>>> > llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll
>>> > Modified:
>>> > llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp
>>> >
>>> > Modified: llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp?rev=215962&r1=215961&r2=215962&view=diff
>>> >
>>> ==============================================================================
>>> > --- llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp (original)
>>> > +++ llvm/trunk/lib/Target/X86/X86FloatingPoint.cpp Mon Aug 18 21:09:57
>>> 2014
>>> > @@ -1654,6 +1654,9 @@ void FPS::setKillFlags(MachineBasicBlock
>>> >
>>> > for (MachineBasicBlock::reverse_iterator I = MBB.rbegin(), E =
>>> MBB.rend();
>>> > I != E; ++I) {
>>> > + if (I->isDebugValue())
>>> > + continue;
>>> > +
>>> > BitVector Defs(8);
>>> > SmallVector<MachineOperand *, 2> Uses;
>>> > MachineInstr &MI = *I;
>>> >
>>> > Added: llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll?rev=215962&view=auto
>>> >
>>> ==============================================================================
>>> > --- llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll (added)
>>> > +++ llvm/trunk/test/CodeGen/X86/fpstack-debuginstr-kill.ll Mon Aug 18
>>> 21:09:57 2014
>>> > @@ -0,0 +1,71 @@
>>> > +; RUN: llc < %s -mcpu=generic -mtriple=i386-apple-darwin
>>> -no-integrated-as
>>>
>>> Generally we've made a habit of checking in the user-level source
>>> along with the IR for debug info tests, to make them easier to
>>> maintain in the future, should the schema change greatly, etc. But I
>>> realize this is swift code (guessing by the 'sw' prefix in the label
>>> names) & with the swift compiler not being part of the LLVM project (&
>>> still changing a lot) adding swift code here's probably not much help.
>>>
>>> At least a bit more detailed documentation about which part of all of
>>> this code is relevant might be helpful?
>>>
>>> Could the code be simplified (generally we try to simplify the
>>> input/source code, rather than hand-simplify the IR, in these debug
>>> info tests - but since there's no source, the IR could be
>>> hand-simplified (but I'd suggest trying to simplify the input source
>>> as much as possible first - since that'll also simplify the metadata,
>>> too)). Perhaps there's a C++ reproduction you could use, in which case
>>> we wouldn't have to worry about the swiftyness of the code and it
>>> might be more obvious?
>>>
>>>
>> The source code is actually a c++ program, not a swift code, so I can
>> check it in if that's necessary.
>>
>> 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?
>>
>
> Still seems like an awful lot of source code to tickle this - is it not
> possible to further simplify it?
>
>
I tried reducing the size of the source code, but it still has 41 lines
(please see attached file). The bug is triggered only when the debug
instruction has an FP register operand that is the last use, which isn't
something that happens often. I'll continue trying to reduce the size, but
I don't how small I can make it.
> Also, where do the source c++ programs live in the test directory?
>>
>
> usually we just quote it in the test case (see other debug info test cases
> for examples)
>
>
>>
>>
>>> Is this test just checking we don't crash? (I don't see any FileCheck,
>>> etc) "does not crash" isn't a great thing to test for. There must've
>>> been some behavior that was expected of this code other than "does not
>>> crash" - perhaps we could/should test for that behavior?
>>>
>>>
>> 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?
>>
>
> Not quite sufficient, I don't think.
>
> The question is about what the expected behavior was that wasn't being
> tested.
>
> 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.
>
>
So the test should satisfy these three requirements:
1. It crashes if the lines I added in r215962 are removed.
2. If I don't run FPS::setKillFlags, llc will generate a different assembly
output.
3. The test (the source code and the .ll file including debug info) should
be small.
Right now the attached test doesn't satisfy 2. setKillFlags adds kill flags
only when they are missing, and most instructions already have the kill
flags added to their operands.
I haven't tried adding debug intrinsics to the existing tests (e.g.,
CodeGen/X86/inline-asm-fpstack.ll) yet.
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.
>
> (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)
>
> - David
>
>
>>
>>
>>
>>
>>> > +
>>> > + at g1 = global double 0.000000e+00, align 8
>>> > + at g2 = global i32 0, align 4
>>> > +
>>> > +define void @_Z16fpuop_arithmeticjj(i32, i32) {
>>> > +entry:
>>> > + switch i32 undef, label %sw.bb.i1921 [
>>> > + ]
>>> > +
>>> > +sw.bb261: ; preds = %entry,
>>> %entry
>>> > + unreachable
>>> > +
>>> > +sw.bb.i1921: ; preds = %if.end504
>>> > + switch i32 undef, label %if.end511 [
>>> > + i32 1, label %sw.bb27.i
>>> > + ]
>>> > +
>>> > +sw.bb27.i: ; preds =
>>> %sw.bb.i1921
>>> > + %conv.i.i1923 = fpext float undef to x86_fp80
>>> > + br label %if.end511
>>> > +
>>> > +if.end511: ; preds =
>>> %sw.bb27.i, %sw.bb13.i
>>> > + %src.sroa.0.0.src.sroa.0.0.2280 = phi x86_fp80 [ %conv.i.i1923,
>>> %sw.bb27.i ], [ undef, %sw.bb.i1921 ]
>>> > + switch i32 undef, label %sw.bb992 [
>>> > + i32 3, label %sw.bb735
>>> > + i32 18, label %if.end41.i2210
>>> > + ]
>>> > +
>>> > +sw.bb735: ; preds = %if.end511
>>> > + %2 = call x86_fp80 asm sideeffect "frndint",
>>> "={st},0,~{dirflag},~{fpsr},~{flags}"(x86_fp80
>>> %src.sroa.0.0.src.sroa.0.0.2280)
>>> > + unreachable
>>> > +
>>> > +if.end41.i2210: ; preds = %if.end511
>>> > + call void @llvm.dbg.value(metadata !{x86_fp80
>>> %src.sroa.0.0.src.sroa.0.0.2280}, i64 0, metadata !20)
>>> > + unreachable
>>> > +
>>> > +sw.bb992: ; preds = %if.end511
>>> > + ret void
>>> > +}
>>> > +
>>> > +declare void @llvm.dbg.value(metadata, i64, metadata)
>>> > +
>>> > +!llvm.dbg.cu = !{!0}
>>> > +!llvm.module.flags = !{!24, !25}
>>> > +!0 = metadata !{i32 786449, metadata !1, i32 4, metadata !"clang
>>> version 3.6.0 (http://llvm.org/git/clang
>>> 8444ae7cfeaefae031f8fedf0d1435ca3b14d90b) (http://llvm.org/git/llvm
>>> 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]
>>> > +!1 = metadata !{metadata !"fpu_ieee.cpp", metadata !"x87stackifier"}
>>> > +!2 = metadata !{}
>>> > +!3 = metadata !{metadata !4}
>>> > +!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]
>>> > +!5 = metadata !{metadata !"f1.cpp", metadata !"x87stackifier"}
>>> > +!6 = metadata !{i32 786473, metadata !5} ; [
>>> DW_TAG_file_type ] [x87stackifier/f1.cpp]
>>> > +!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 ]
>>> > +!8 = metadata !{null, metadata !9, metadata !9}
>>> > +!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]
>>> > +!10 = metadata !{metadata !11, metadata !12, metadata !13, metadata
>>> !18, metadata !20}
>>> > +!11 = metadata !{i32 786689, metadata !4, metadata !"", metadata !6,
>>> i32 16777227, metadata !9, i32 0, i32 0} ; [ DW_TAG_arg_variable ] [line 11]
>>> > +!12 = metadata !{i32 786689, metadata !4, metadata !"", metadata !6,
>>> i32 33554443, metadata !9, i32 0, i32 0} ; [ DW_TAG_arg_variable ] [line 11]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!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]
>>> > +!21 = metadata !{metadata !22, metadata !23}
>>> > +!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]
>>> > +!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]
>>> > +!24 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
>>> > +!25 = metadata !{i32 2, metadata !"Debug Info Version", i32 1}
>>> >
>>> >
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at cs.uiuc.edu
>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>> _______________________________________________
>>> 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/20140903/5854b9b0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fpstack-debuginstr-kill2.cpp
Type: text/x-c++src
Size: 789 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140903/5854b9b0/attachment.cpp>
More information about the llvm-commits
mailing list