[llvm-commits] [PATCH] Fix assertion failure with shufflevectors on ARM

[James Molloy]

Hi Eli,

Great, thanks. While seemingly extremely obvious it's this sort of
contract that I was missing.

I'll have another go at getting to the real root cause.

Cheers,

James

On Tue, 2012-08-14 at 02:46 +0100, Eli Friedman wrote:
> On Mon, Aug 13, 2012 at 6:43 AM, James Molloy <James.Molloy at arm.com> wrote:
> > Hi,
> >
> > The attached patch fixes an assertion failure in the DAGCombiner. The
> > testcase is simply three shufflevectors.
> >
> > The DAGCombiner tries to optimise a BUILD_VECTOR by checking if it
> > consists purely of get_vector_elts from one or two source vectors. If
> > so, it either makes a concat_vectors node or a shufflevector node.
> >
> > However, it doesn't check the element type width of the underlying
> > vector, so if you have this sequence:
> >
> > [0] v4i16 = ...
> > [1] i32 = extract_vector_elt [0]
> > [2] i32 = extract_vector_elt [0]
> > [3] v16i8 = BUILD_VECTOR [1], [2], ...
> >
> > It will attempt to:
> >
> > [0] v4i16 = ...
> > [1] v16i8 = concat_vectors [0], ...
> >
> > Where this is actually invalid because the element width is completely
> > different. This causes an assertion failure down the line.
> >
> > My fix for this, I'm reasonably happy with. However the fix (bail out if
> > element width is incorrect) causes another bug further down the line.
> >
> > You have this code:
> >
> > [0] v4i16 = ...
> > [1] i16 = get_vector_elt [0]
> > [2] i8 = truncate [0]
> > [3] v16i8 = BUILD_VECTOR [2], ...
> >
> > It gets legalized (because vectors of i8 are not valid on ARM NEON), to
> > something like:
> >
> > [0] v4i16 = ...
> > [1] i16 = get_vector_elt [0]
> > [2] v16i8 = BUILD_VECTOR [1], ...
> > [3] i8 = truncate [0]
> >
> > Note that [3] is an orphan node. This orphan node is traversed in
> > LegalizeDAG() and causes an assertion failure because it is not legal.
> >
> > This is where my SelectionDAG-fu gets rusty. I believe it is due to
> > DAG.RemoveDeadNodes() not having fired, which should remove that node
> > completely and stop the combiner from attempting to legalize it.
> >
> > The patch attached does that simply by moving the call to
> > RemoveDeadNodes() inside its parent loop.
> >
> > I'm not convinced this is the neatest solution however - I'm convinced
> > there must be some reason why that orphan node isn't being removed at
> > the right place. But I don't know some of the internal contracts in
> > SelectionDAG too well, so I'm not sure.
> >
> > Could someone with more knowledge about SelectionDAG please look at
> > whether there is a contract being broken somewhere as opposed to a dead
> > node not being cleaned up quickly enough?
>
> There should be no nodes with illegal types upon entrance to
> LegalizeDAG, and LegalizeDAG should not be producing any nodes with
> illegal types.  If shuffling around the call to RemoveDeadNodes
> somehow makes a node with an illegal type disappear, there's a bug
> elsewhere.
>
> -Eli
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.