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

[James Molloy]

Hi Eli,

Thanks for that. I've attached another patch, written by Stepan
Dyatkovskiy, that fixes the primary issue in a different way (bitcast
instead of bail out) and because of this I cannot now reproduce the
secondary issue (which was dead nodes remaining live).

Could you please review the attached patch? Is it OK to commit?

[As a sidenote, I debugged the secondary problem down to: A
shufflevector can produce code with illegal types when expanded. This
appears to be controlled for other, similar instructions by having them
expand in LegalizeVectorOps (Which has another LegalizeTypes pass after
it). But shufflevector isn't dealt with there, so illegal types are
generated with no LegalizeTypes pass after them.

I have a patch to fix this, but I cannot actually produce a testcase
that now goes down that codepath :(]

Cheers, and thanks Stepan!

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crash-shufflevector.diff
Type: text/x-patch
Size: 3320 bytes
Desc: crash-shufflevector.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120814/56c02356/attachment.bin>