Re: [PATCH take #2] Fold truncations of left shifts in match.pd

[Richard Biener <richard.guenther@gmail.com>]

On Sun, Jun 5, 2022 at 1:12 PM Roger Sayle <roger@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  <roger@nextmovesoftware.com>
>             Richard Biener  <rguenther@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.guenther@gmail.com>
> > Sent: 02 June 2022 12:03
> > To: Roger Sayle <roger@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 <roger@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
> > > --
> > >
> > >