[PATCH] Fix PR10475

Michael Liao michael.liao at intel.com
Fri Feb 22 13:36:46 PST 2013


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





More information about the llvm-commits mailing list