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

[Eli Friedman]

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