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

[James Molloy]

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?

Cheers,

James

-- 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.patch
Type: text/x-patch
Size: 2437 bytes
Desc: crash-shufflevector.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120813/1eb245a7/attachment.bin>