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

[Stepan Dyatkovskiy]

ping?
Stepan Dyatkovskiy wrote:
> ping
> Stepan Dyatkovskiy wrote:
>> Hi all!
>> Due to Duncan remarks for r162195:
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120820/148595.html
>>
>>
>> I reverted this patch and proceed debugging the problem. Now it seems
>> that I found proper places for fix.
>>
>> I found that crashes, described by James in initial post are happened
>> due to two bugs.
>>
>> Bug #1. We insert wrong concat_vectors operation while expanding
>> build_vector in DAGCombiner::visitBUILD_VECTOR:
>> v16i8 concat_vectors v4i16, v4i16
>> That happens due to improperly formed condition: when we can expand
>> build_vector and when we can't. We support widening of vectors which are
>> half the size of the output registers.
>> In TOT you can found the check:
>> if (VecIn1.getValueType().getSizeInBits()*2 != VT.getSizeInBits())
>>    return SDValue()
>> I think, it is wrong, since we should check number of elements instead
>> of bit width. This check doesn't take into account input and output
>> vectors number. It also takes into account bit width, but it doesn't
>> matter: if we produce illegal operation (input width != output width) it
>> will be legalized later.
>>
>> Bug #2. Fired when LegalizeDAG tries to produce extract_vector_elt with
>> operands that are build_vector nodes. getNode tries to return
>> build_vector's operand in that case, but it also inserts the nodes, that
>> performs high bits cleaning from garbage:
>> Node0: i32 some-node-here
>> Node1: v2i16 build_vector Node0, Node0
>>
>> // Current nodes sequence inserted when we invoke
>> // getNode(EXTRACT_VECTOR_ELT, dl, i32, Node0, idx=0)
>> Node1: i16 truncate Node0
>> Node2: i32 any_extend Node1
>> // return Node2 instead of extract_vector_elt node.
>>
>> I'm 99,9(9)% sure, that it is wrong way, due to next reasons:
>>   1. 16 may be illegal type (even if v2i16 is legal), so, at least you
>> need to check  CombineLevel, it should be BeforeLegalizeTypes. In
>> another case you will crash the LegalizeDAG.
>>   2. What if initially elements were i8 and then was transformed since
>> target is 32bit machine?
>>   Node0: i8 some-node-here
>>   Node1: v2i8 build_vector Node0, NodeItem1, NodeItem2, NodeItem3
>>   After type legalization we got current state:
>>   Node0: i32 some-node-here
>>   Node1: v2i16 build_vector Node0, NodeItem1, NodeItem2, NodeItem3
>>   But you can't determine initial type (was it 8bit or may be 4bit?) on
>> this stage. So 'i16 = truncate Node0' will not guarantee high bits
>> cleanness.
>>   3. build_vector operands were already extended to
>>   machine word width (32 bit in our case) with some way. We needn't do
>>   the same job twice.
>>
>> Please find the fix in attachment.
>>
>> -Stepan.
>>
>> Eli Friedman wrote:
>>> On Wed, Aug 15, 2012 at 2:55 AM, Stepan Dyatkovskiy
>>> <stpworld at narod.ru> wrote:
>>>> Hi Eli,
>>>> Please review the patch with bitcasting after concat_vectors.
>>>
>>> Looks fine.
>>>
>>> -Eli
>>>
>>
>