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

[Eli Friedman]

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