[llvm] r209242 - [ARM64] PR19792: Fix cycle in DAG after performPostLD1Combine
David Blaikie
dblaikie at gmail.com
Tue May 20 17:19:34 PDT 2014
On Tue, May 20, 2014 at 4:50 PM, Adam Nemet <anemet at apple.com> wrote:
> I think I agree in general (and in fact I considered adding a FileCheck because I know that’s preferred). I am not sure I agree in this particular case though.
>
> Here we basically want to test that the post-incremented version of a load instruction is "not generated if it creates a cycle in the DAG". Now I can test that we don’t generate this version of the instruction and the test case is luckily minimized enough so that we hopefully won’t have another valid case of that. But it seems to me that it’s more targeted to actually test for the lack of cycle in the DAG given this input. (This is what I was trying to describe in the comment in the test.)
>
> I guess another way to consider this is that the checking is implicit to llc. Since llc already detects this condition with a debug build we don’t need the explicit check.
>
> I also consider these minimized testcases more like torture test than unit tests.
>
> Anyhow, if this is not convincing enough, I can of course try to add something that is hopefully sufficiently robust.
Not sure I have enough domain knowledge here to agree/disagree -
figured I'd just mention it. Perhaps someone else who knows more about
the area can weigh in and agree/disagree with you there (or not).
- Dave
>
> Adam
>
> On May 20, 2014, at 4:07 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> As an aside, tests that simplify verify that something doesn't crash
>> are usually a bit of a code smell from my perspective.
>>
>> If the code previously crashed - what was the missing behavioral test
>> case? There must be some behavior that was intended (other than "don't
>> crash") that we weren't testing for and should be now testing for...
>>
>> On Tue, May 20, 2014 at 3:52 PM, Adam Nemet <anemet at apple.com> wrote:
>>> On May 20, 2014, at 3:49 PM, Alexey Samsonov <samsonov at google.com> wrote:
>>>
>>>
>>> On Tue, May 20, 2014 at 2:47 PM, Adam Nemet <anemet at apple.com> wrote:
>>>>
>>>> Author: anemet
>>>> Date: Tue May 20 16:47:07 2014
>>>> New Revision: 209242
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=209242&view=rev
>>>> Log:
>>>> [ARM64] PR19792: Fix cycle in DAG after performPostLD1Combine
>>>>
>>>> Povray and dealII currently assert with "Overran sorted position" in
>>>> AssignTopologicalOrder. The problem is that performPostLD1Combine can
>>>> introduce cycles.
>>>>
>>>> Consider:
>>>>
>>>> (insert_vector_elt (INSERT_SUBREG undef,
>>>> (load (add %vreg0, Constant<8>), undef),
>>>> <= A
>>>> TargetConstant<2>),
>>>> (load %vreg0, undef),
>>>> <= B
>>>> Constant<1>)
>>>>
>>>> This is turned into a LD1LANEpost node. However the address in A is not a
>>>> valid user of the post-incremented address of B in LD1LANEpost.
>>>>
>>>> Added:
>>>> llvm/trunk/test/CodeGen/ARM64/indexed-vector-ldst-2.ll
>>>> Modified:
>>>> llvm/trunk/lib/Target/ARM64/ARM64ISelLowering.cpp
>>>>
>>>> Modified: llvm/trunk/lib/Target/ARM64/ARM64ISelLowering.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM64/ARM64ISelLowering.cpp?rev=209242&r1=209241&r2=209242&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Target/ARM64/ARM64ISelLowering.cpp (original)
>>>> +++ llvm/trunk/lib/Target/ARM64/ARM64ISelLowering.cpp Tue May 20 16:47:07
>>>> 2014
>>>> @@ -7298,6 +7298,7 @@ static SDValue performPostLD1Combine(SDN
>>>> }
>>>>
>>>> SDValue Addr = LD->getOperand(1);
>>>> + SDValue Vector = N->getOperand(0);
>>>> // Search for a use of the address operand that is an increment.
>>>> for (SDNode::use_iterator UI = Addr.getNode()->use_begin(), UE =
>>>> Addr.getNode()->use_end(); UI != UE; ++UI) {
>>>> @@ -7310,6 +7311,10 @@ static SDValue performPostLD1Combine(SDN
>>>> // would create a cycle.
>>>> if (User->isPredecessorOf(LD) || LD->isPredecessorOf(User))
>>>> continue;
>>>> + // Also check that add is not used in the vector operand. This would
>>>> also
>>>> + // create a cycle.
>>>> + if (User->isPredecessorOf(Vector.getNode()))
>>>> + continue;
>>>>
>>>> // If the increment is a constant, it must match the memory ref size.
>>>> SDValue Inc = User->getOperand(User->getOperand(0) == Addr ? 1 : 0);
>>>> @@ -7324,7 +7329,7 @@ static SDValue performPostLD1Combine(SDN
>>>> SmallVector<SDValue, 8> Ops;
>>>> Ops.push_back(LD->getOperand(0)); // Chain
>>>> if (IsLaneOp) {
>>>> - Ops.push_back(N->getOperand(0)); // The vector to be inserted
>>>> + Ops.push_back(Vector); // The vector to be inserted
>>>> Ops.push_back(N->getOperand(2)); // The lane to be inserted in the
>>>> vector
>>>> }
>>>> Ops.push_back(Addr);
>>>>
>>>> Added: llvm/trunk/test/CodeGen/ARM64/indexed-vector-ldst-2.ll
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM64/indexed-vector-ldst-2.ll?rev=209242&view=auto
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/CodeGen/ARM64/indexed-vector-ldst-2.ll (added)
>>>> +++ llvm/trunk/test/CodeGen/ARM64/indexed-vector-ldst-2.ll Tue May 20
>>>> 16:47:07 2014
>>>> @@ -0,0 +1,40 @@
>>>> +; RUN: llc %s
>>>
>>>
>>> ^^
>>> This line would create file test/CodeGen/ARM64/indexed-vector-ldst-2.s in
>>> the source tree. I've mailed a fix in r209252.
>>>
>>>
>>> Ah! Sorry about that and thanks for the fix!
>>>
>>> Adam
>>>
>>>
>>>>
>>>> +
>>>> +; This used to assert with "Overran sorted position" in
>>>> AssignTopologicalOrder
>>>> +; due to a cycle created in performPostLD1Combine.
>>>> +
>>>> +target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
>>>> +target triple = "arm64-apple-ios7.0.0"
>>>> +
>>>> +; Function Attrs: nounwind ssp
>>>> +define void @f(double* %P1) #0 {
>>>> +entry:
>>>> + %arrayidx4 = getelementptr inbounds double* %P1, i64 1
>>>> + %0 = load double* %arrayidx4, align 8, !tbaa !1
>>>> + %1 = load double* %P1, align 8, !tbaa !1
>>>> + %2 = insertelement <2 x double> undef, double %0, i32 0
>>>> + %3 = insertelement <2 x double> %2, double %1, i32 1
>>>> + %4 = fsub <2 x double> zeroinitializer, %3
>>>> + %5 = fmul <2 x double> undef, %4
>>>> + %6 = extractelement <2 x double> %5, i32 0
>>>> + %cmp168 = fcmp olt double %6, undef
>>>> + br i1 %cmp168, label %if.then172, label %return
>>>> +
>>>> +if.then172: ; preds = %cond.end90
>>>> + %7 = tail call i64 @llvm.objectsize.i64.p0i8(i8* undef, i1 false)
>>>> + br label %return
>>>> +
>>>> +return: ; preds = %if.then172,
>>>> %cond.end90, %entry
>>>> + ret void
>>>> +}
>>>> +
>>>> +; Function Attrs: nounwind readnone
>>>> +declare i64 @llvm.objectsize.i64.p0i8(i8*, i1) #1
>>>> +
>>>> +attributes #0 = { nounwind ssp "less-precise-fpmad"="false"
>>>> "no-frame-pointer-elim"="false" "no-infs-fp-math"="false"
>>>> "no-nans-fp-math"="false" "stack-protector-buffer-size"="8"
>>>> "unsafe-fp-math"="false" "use-soft-float"="false" }
>>>> +attributes #1 = { nounwind readnone }
>>>> +
>>>> +!1 = metadata !{metadata !2, metadata !2, i64 0}
>>>> +!2 = metadata !{metadata !"double", metadata !3, i64 0}
>>>> +!3 = metadata !{metadata !"omnipotent char", metadata !4, i64 0}
>>>> +!4 = metadata !{metadata !"Simple C/C++ TBAA"}
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>>
>>>
>>> --
>>> Alexey Samsonov, Mountain View, CA
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>
More information about the llvm-commits
mailing list