[llvm-commits] [PATCH] Bug 10897 - Seg fault during NEON compilation with short3/4 division

[Eli Friedman]

On Fri, Sep 9, 2011 at 9:53 AM, James Molloy <james.molloy at arm.com> wrote:
> Hi Eli,
>
> Thanks for the quick response - I'd noticed you'd commented on the issue but as
> you hadn't assigned it, I thought it was dormant.
>
> I admit that my experience in the SelectionDAG code is lacking - could you
> please explain (briefly) what the correct fix is, for my information?

Your fix is in the right direction... but you're going to end up
triggering the exact opposite problem: before type legalization first
runs, the undef's will be wider than the inserted elements.

I have a fix I will be committing shortly; thanks for the testcase.

-Eli

> Cheers,
>
> James
>
>> -----Original Message-----
>> From: Eli Friedman [mailto:eli.friedman at gmail.com]
>> Sent: 09 September 2011 17:45
>> To: James Molloy
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [PATCH] Bug 10897 - Seg fault during NEON
>> compilation with short3/4 division
>>
>> On Fri, Sep 9, 2011 at 7:43 AM, James Molloy <james.molloy at arm.com> wrote:
>> > Hi,
>> >
>> > The attached patch fixes bug 10897.
>> >
>> > The issue was in the DAGCombiner, when constant-folding
>> "insert_vector_emt"
>> > nodes on a vector that had been size-extended (<3 x int> -> <4 x int> for
>> > example).
>> >
>> > The DAGCombiner was inserting UNDEFs for all values in the vector, and
>> the
>> > folded insert_vector_emt nodes replace those UNDEFs with actual content.
>> But the
>> > padding at the end, in the case where the original vector was not a power
>> of 2,
>> > isn't changed.
>> >
>> > This is fine, apart from the type that was being used for UNDEF was the
>> type of
>> > the vector element, which may not be legal. As DAGTypeLegalizer has
>> already run
>> > by this point, inserting illegal nodes causes assertion failures later
>> down the
>> > line.
>> >
>> > The fix is to ensure the type of the UNDEFs are legal types:
>> >
>> > +    if (!isTypeLegal(EltVT))
>> > +        EltVT = TLI.getTypeToTransformTo(*DAG.getContext(), EltVT);
>> > +    assert(isTypeLegal(EltVT) && "Vector element type was not legal!");
>>
>> This fix is not right; it will just end up exploding in the same way
>> on a different testcase.
>>
>> I've already taken a look at the bug; I wanted to briefly discuss it
>> with Dan first before committing a fix.
>>
>> > ----
>> >
>> > Aside: The documentation for TargetLowering::getTypeToTransformTo()
>> states that
>> > if the input parameter is legal, the function is equivalent to the
>> identity
>> > function. I should therefore not need the "isTypeLegal" check on the line
>> above.
>> >
>> > Changing this however broke a lot of codegen tests, both in the ARM and
>> X86
>> > backends. Does anyone know why that might be the case? For the X86 test I
>> looked
>> > at, getTypeToTransformTo appeared to be changing an i64 to a v2i64
>> (IIRC).
>>
>> That's strange... no idea.
>>
>> -Eli
>
>
>
>