[PATCH] Fix PR10475

[Nadav Rotem]

Hi Michael, 

Sorry for the delay.  LGTM. 

Thanks,
Nadav

On Mar 1, 2013, at 9:02 AM, Michael Liao <michael.liao at intel.com> wrote:

> ping
> 
> - michael
> 
> On Wed, 2013-02-27 at 11:56 -0800, Michael Liao wrote:
>> Hi Nadav
>> 
>> It's possible but that will duplicate the logic handling vector type for
>> each target. There are several options.
>> 
>> + current approach: rename the original getShiftAmountTy() to
>> getScalarShiftAmountTy() and enhance the logic in getShiftAmountTy() to
>> handle vector type. Out-of-tree targets need to change their name only.
>> 
>> + keep the name of getShiftAmountTy() and enhance its logic to handle
>> vector type. All targets will duplicate that logic. Out-of-tree targets
>> still need that change duplicating the logic.
>> 
>> + keep the name of getShiftAmountTy() and add another new method to
>> handle vector type. We have to fix all getShiftAmountTy() occurrences in
>> TICG plus all targets if they use them for vector shift.
>> 
>> The current approach sounds to me a cleaner way to put new constraints
>> on vector shift ops. Comments?
>> 
>> BTW, they will break if out-of-tree targets rely on scalar shift amount
>> on vector shift.
>> 
>> Yours
>> - Michael
>> 
>> 
>> On Wed, 2013-02-27 at 11:32 -0800, Nadav Rotem wrote:
>>> The change looks correct. I am a little worried about out-of-tree
>>> targets. Would it be possible to keep the original name of
>>> GetShiftAmountTy ?
>>> 
>>> 
>>> Thanks,
>>> Nadav
>>> 
>>> On Feb 27, 2013, at 10:59 AM, Michael Liao <michael.liao at intel.com>
>>> wrote:
>>> 
>>>> Hi Nadav
>>>> 
>>>> Continue the approach we discussed, after fixing more places in
>>>> TICG,
>>>> PR10475 is fixed. Please review the attached patch.
>>>> 
>>>> Yours
>>>> - Michael
>>>> 
>>>> On Fri, 2013-02-22 at 13:36 -0800, Michael Liao wrote:
>>>>> Hi Nadav
>>>>> 
>>>>> On Fri, 2013-02-22 at 13:30 -0800, Nadav Rotem wrote:
>>>>>> Hi Michael, 
>>>>>> 
>>>>>> I prefer not to work around problems when we can fix them.  You
>>>>>> can fix TLI.getShiftAmountTy to return vectors. It is obvious to
>>>>>> me that the shift amount should match the shifted data type.
>>>>>> This is the way it is in the LLVM IR, and this is the way it is
>>>>>> in most parts of SelectionDAG. We need to fix this. You don't
>>>>>> have to do it yourself, but please don't add more workarounds.
>>>>> 
>>>>> That's my first attempt to solve this issue as I have the same
>>>>> feeling
>>>>> ISD::SHL should to be consistent with LLVM IR. However, the change
>>>>> needs
>>>>> more effort as EVT instead of MVT will be returned from
>>>>> TLI.getShiftAmountTy() and lots of places in TICG assumes that
>>>>> being an
>>>>> MVT.
>>>>> 
>>>>> Technically, allowing a scalar shift amount in vector shift is a
>>>>> legal
>>>>> type in many targets, such as ARM NEON. Just ask thoughts where we
>>>>> consider relax ISD::SHL. Anyway, I have no preference on either
>>>>> approach.
>>>>> 
>>>>> Yours
>>>>> - Michael
>>>>>> 
>>>>>> Thanks,
>>>>>> Nadav
>>>>>> 
>>>>>> On Feb 21, 2013, at 9:36 AM, Michael Liao
>>>>>> <michael.liao at intel.com> wrote:
>>>>>> 
>>>>>>> Hi Nadav
>>>>>>> 
>>>>>>> On Thu, 2013-02-21 at 09:12 -0800, Nadav Rotem wrote:
>>>>>>>> I think that we should make sure that the shifted value and
>>>>>>>> the amount are either both vectors or both scalars.  I
>>>>>>>> looked at it in the past and wanted to add assertions, but
>>>>>>>> the Cell backend relied on this behavior. But now that the
>>>>>>>> Cell backend is gone we can add the assertions and catch all
>>>>>>>> of the places that violate this rule. 
>>>>>>> 
>>>>>>> That will break all shifts generated within TICG. All code
>>>>>>> there call
>>>>>>> TLI.getShiftAmountTy() to create the shift amount operand. But
>>>>>>> all
>>>>>>> shifts generated in TICG fall into vector-scalar shift form.
>>>>>>> There's
>>>>>>> another issue by forcing that. It will also break lots of
>>>>>>> assertions
>>>>>>> assuming ShiftAmountTy() to be MVT instead of EVT. With DAG
>>>>>>> preparation,
>>>>>>> the vector shift amount may have to an EVT (illegal type
>>>>>>> mostly.)
>>>>>>> 
>>>>>>> Allowing shift-amount to be scalar or the same vector type if
>>>>>>> the 1st
>>>>>>> operand is an integer vector may has the minor impact for the
>>>>>>> existing
>>>>>>> code comparing ensuring them to be either both vectors or both
>>>>>>> scalars.
>>>>>>> What's your thoughts?
>>>>>>> 
>>>>>>> Yours
>>>>>>> - Michael
>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Nadav 
>>>>>>>> 
>>>>>>>> On Feb 20, 2013, at 11:52 PM, Michael Liao
>>>>>>>> <michael.liao at intel.com> wrote:
>>>>>>>> 
>>>>>>>>> Hi All,
>>>>>>>>> 
>>>>>>>>> The root cause of PR10475 is the X86 vector shift lowering
>>>>>>>>> logic assumes
>>>>>>>>> the shift amount operand to be a vector. But all shift
>>>>>>>>> nodes generated
>>>>>>>>> by generic DAG combination/optimization, e.g. (Z-X) == X
>>>>>>>>> --> Z == X<<1,
>>>>>>>>> have scalar shift amount.
>>>>>>>>> 
>>>>>>>>> The documentation on ISD::SHL doesn't explicitly state
>>>>>>>>> that there's no
>>>>>>>>> such restriction on scalar or vector. (You could guess
>>>>>>>>> that since it
>>>>>>>>> mentioned it should be of type TLI.getShiftAmountTy(),
>>>>>>>>> which always
>>>>>>>>> return scalar type on all targets so far.) This may cause
>>>>>>>>> misunderstanding as 'shl' in LLVM IR does have the
>>>>>>>>> restriction as "Both
>>>>>>>>> arguments to the ‘shl‘ instruction must be the same
>>>>>>>>> integer or vector of
>>>>>>>>> integer type." I'd like to revise the document if we think
>>>>>>>>> it's
>>>>>>>>> necessary.
>>>>>>>>> 
>>>>>>>>> BTW, another thought is that if shift-amount is allowed to
>>>>>>>>> be a scalar,
>>>>>>>>> we may add a generic DAG combination to detect cases where
>>>>>>>>> a vector
>>>>>>>>> shift-amount could be treated as a scalar. This could save
>>>>>>>>> all targets
>>>>>>>>> with similar vector-scalar-shift ops.
>>>>>>>>> 
>>>>>>>>> Yours
>>>>>>>>> - Michael
>>>>>>>>> <0001-Fix-PR10475.patch>_______________________________________________
>>>>>>>>> llvm-commits mailing list
>>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>>> <0001-Fix-PR10475.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130301/d22549c3/attachment.html>