[llvm] r215962 - [X86, X87 stackifier] Do not mark an operand of a debug instruction as kill.

David Blaikie dblaikie at gmail.com
Wed Sep 3 13:29:44 PDT 2014


On Wed, Sep 3, 2014 at 1:21 PM, Akira Hatanaka <ahatanak at gmail.com> wrote:

> 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.
>

If you end up with a hand-crafted assembly test (like the existing tests)
you might also be able to address (2) by omitting the kill flags
deliberately, rather than having to construct a case in which they are
naturally missing.


>
> 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/b3dc5079/attachment.html>


More information about the llvm-commits mailing list