[PATCH] Fix PR10475

[Michael Liao]

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>
>