[PATCH] Fix PR10475

[Michael Liao]

Thanks, committed as r176364.

- michael

On Fri, 2013-03-01 at 10:18 -0800, Nadav Rotem wrote:
> 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>
>