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

[James Molloy]

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?

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