[PATCH] Fix PR10475

[Michael Liao]

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