On Sun, Jun 5, 2022 at 1:12 PM Roger Sayle <ro...@nextmovesoftware.com> wrote:
>
>
> Hi Richard,
> Many thanks for taking the time to explain how vectorization is supposed
> to work. I now see that vect_recog_rotate_pattern in tree-vect-patterns.cc
> is supposed to handle lowering of rotations to (vector) shifts, and
> completely agree that adding support for signed types (using appropriate
> casts to unsigned_type_for and casting the result back to the original
> signed type) is a better approach to avoid the regression of pr98674.c.
>
> I've also implemented your suggestions of combining the proposed new
> (convert (lshift @1 INTEGER_CST@2)) with the existing one, and at the
> same time including support for valid shifts greater than the narrower
> type, such as (short)(x << 20), to constant zero. Although this optimization
> is already performed during the tree-ssa passes, it's convenient to
> also catch it here during constant folding.
>
> This revised patch has been tested on x86_64-pc-linux-gnu with
> make bootstrap and make -k check, both with and without
> --target_board=unix{-m32}, with no new failures. Ok for mainline?
OK.
Thanks,
Richard.
> 2022-06-05 Roger Sayle <ro...@nextmovesoftware.com>
> Richard Biener <rguent...@suse.de>
>
> gcc/ChangeLog
> * match.pd (convert (lshift @1 INTEGER_CST@2)): Narrow integer
> left shifts by a constant when the result is truncated, and the
> shift constant is well-defined.
> * tree-vect-patterns.cc (vect_recog_rotate_pattern): Add
> support for rotations of signed integer types, by lowering
> using unsigned vector shifts.
>
> gcc/testsuite/ChangeLog
> * gcc.dg/fold-convlshift-4.c: New test case.
> * gcc.dg/optimize-bswaphi-1.c: Update found bswap count.
> * gcc.dg/tree-ssa/pr61839_3.c: Shift is now optimized before VRP.
> * gcc.dg/vect/vect-over-widen-1-big-array.c: Remove obsolete tests.
> * gcc.dg/vect/vect-over-widen-1.c: Likewise.
> * gcc.dg/vect/vect-over-widen-3-big-array.c: Likewise.
> * gcc.dg/vect/vect-over-widen-3.c: Likewise.
> * gcc.dg/vect/vect-over-widen-4-big-array.c: Likewise.
> * gcc.dg/vect/vect-over-widen-4.c: Likewise.
>
>
> Thanks again,
> Roger
> --
>
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: 02 June 2022 12:03
> > To: Roger Sayle <ro...@nextmovesoftware.com>
> > Cc: GCC Patches <gcc-patches@gcc.gnu.org>
> > Subject: Re: [PATCH] Fold truncations of left shifts in match.pd
> >
> > On Thu, Jun 2, 2022 at 12:55 PM Roger Sayle <ro...@nextmovesoftware.com>
> > wrote:
> > >
> > >
> > > Hi Richard,
> > > > + /* RTL expansion knows how to expand rotates using shift/or. */
> > > > + if (icode == CODE_FOR_nothing
> > > > + && (code == LROTATE_EXPR || code == RROTATE_EXPR)
> > > > + && optab_handler (ior_optab, vec_mode) != CODE_FOR_nothing
> > > > + && optab_handler (ashl_optab, vec_mode) != CODE_FOR_nothing)
> > > > + icode = (int) optab_handler (lshr_optab, vec_mode);
> > > >
> > > > but we then get the vector costing wrong.
> > >
> > > The issue is that we currently get the (relative) vector costing wrong.
> > > Currently for gcc.dg/vect/pr98674.c, the vectorizer thinks the scalar
> > > code requires two shifts and an ior, so believes its profitable to
> > > vectorize this loop using two vector shifts and an vector ior. But
> > > once match.pd simplifies the truncate and recognizes the HImode rotate we
> > end up with:
> > >
> > > pr98674.c:6:16: note: ==> examining statement: _6 = _1 r>> 8;
> > > pr98674.c:6:16: note: vect_is_simple_use: vectype vector(8) short int
> > > pr98674.c:6:16: note: vect_is_simple_use: operand 8, type of def:
> > > constant
> > > pr98674.c:6:16: missed: op not supported by target.
> > > pr98674.c:8:33: missed: not vectorized: relevant stmt not supported: _6
> > > = _1
> > r>> 8;
> > > pr98674.c:6:16: missed: bad operation or unsupported loop bound.
> > >
> > >
> > > Clearly, it's a win to vectorize HImode rotates, when the backend can
> > > perform
> > > 8 (or 16) rotations at a time, but using 3 vector instructions, even
> > > when a scalar rotate can performed in a single instruction.
> > > Fundamentally, vectorization may still be desirable/profitable even when
> > > the
> > backend doesn't provide an optab.
> >
> > Yes, as said it's tree-vect-patterns.cc job to handle this not natively
> > supported
> > rotate by re-writing it. Can you check why vect_recog_rotate_pattern does
> > not
> > do this? Ah, the code only handles !TYPE_UNSIGNED (type) - not sure why
> > though (for rotates it should not matter and for the lowered sequence we can
> > convert to desired signedness to get arithmetic/logical shifts)?
> >
> > > The current situation where the i386's backend provides expanders to
> > > lower rotations (or vcond) into individual instruction sequences, also
> > > interferes
> > with
> > > vector costing. It's the vector cost function that needs to be fixed,
> > > not the
> > > generated code made worse (or the backend bloated performing its own
> > > RTL expansion workarounds).
> > >
> > > Is it instead ok to mark pr98674.c as XFAIL (a regression)?
> > > The tweak to tree-vect-stmts.cc was based on the assumption that we
> > > wished to continue vectorizing this loop. Improving scalar code
> > > generation really shouldn't disable vectorization like this.
> >
> > Yes, see above where the fix needs to be. The pattern will then expose the
> > shift
> > and ior to the vectorizer which then are properly costed.
> >
> > Richard.
> >
> > >
> > >
> > > Cheers,
> > > Roger
> > > --
> > >
> > >
