Re: [PATCH] [i386] Support avx512 vector shift with vector [PR98434]

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

On Wed, Jun 23, 2021 at 9:19 AM Hongtao Liu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Here's the patch I'm going to check in.
>
> The patch will regress pr91838.C with extra options: -march=cascadelake
>
> using T = unsigned char; // or ushort, or uint
> using V [[gnu::vector_size(8)]] = T;
> V f(V x) { return x >> 8 * sizeof(T); }
>
> Basically, the testcase is UB with logic right shift more than 8 bits

I don't see any UB here, it's just x >> 8 but we indeed fail to constant
fold this.  For scalars bit CCP does this, but we seem to lack a
simplification pattern.  There's

/* Optimize x >> x into 0 */
(simplify
 (rshift @0 @0)
  { build_zero_cst (type); })

but nothing looking at element_precision () or using get_nonzero_bits.

I suggest to "ignore" the FAIL and open an enhancement request for this
(or try to add such pattern yourself).

> for unsigned char, but however w/o lashr_optab,  vector operation will
> be lowered to scalar and be optimized by pass_ccp4 in gimple.
> But w/ lashr_optab, it's not optimized and left to the backend, in the
> backend we don't optimize such UB(just like what we did in
> ix86_expand_vec_shift_qihi_constant).
> So I guess maybe gimple should handle such situations to avoid
> "nonoptimal codegen".
>
> On Mon, May 24, 2021 at 5:49 PM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > Hi:
> >   This patch is about to add expanders for vashl<VI12_AVX512BW>,
> > vlshr<VI12_AVX512BW>,
> > vashr<VI1_AVX512BW> and vashr<v32hi,v16hi,v4di,v8di>.
> >
> > Besides there's some assumption in expand_mult_const that mul and
> > add must be available at the same time, but for i386, addv8qi is
> > restricted under TARGET_64BIT, but mulv8qi not, that could cause ICE.
> > So restrict mulv8qi and shiftv8qi under TARGET_64BIT.
> >   Bootstrap and regtested on x86_64-linux-gnu{-m32,} and
> > x86_64-linux-gnu{-m32\ -march=cascadelake,-march=cascadelake}
> >
> > gcc/ChangeLog:
> >
> >         PR target/98434
> >         * config/i386/i386-expand.c (ix86_expand_vec_interleave):
> >         Adjust comments for ix86_expand_vecop_qihi2.
> >         (ix86_expand_vecmul_qihi): Renamed to ..
> >         (ix86_expand_vecop_qihi2): Adjust function prototype to
> >         support shift operation, add static to definition.
> >         (ix86_expand_vec_shift_qihi_constant): Add static to definition.
> >         (ix86_expand_vecop_qihi): Call ix86_expand_vecop_qihi2 and
> >         ix86_expand_vec_shift_qihi_constant.
> >         * config/i386/i386-protos.h (ix86_expand_vecmul_qihi): Deleted.
> >         (ix86_expand_vec_shift_qihi_constant): Deleted.
> >         * config/i386/sse.md (mulv8qi3): Call ix86_expand_vecop_qihi
> >         directly, add condition TARGET_64BIT.
> >         (mul<mode>3): Ditto.
> >         (<insn><mode>3): Ditto.
> >         (vlshr<mode>3): Extend to support avx512 vlshr.
> >         (v<insn><mode>3): New expander for
> >         vashr/vlshr/vashl.
> >         (v<insn>v8qi3): Ditto.
> >         (vashrv8hi3<mask_name>): Renamed to ..
> >         (vashr<mode>3): And extend to support V16QImode for avx512.
> >         (vashrv16qi3): Deleted.
> >         (vashrv2di3<mask_name>): Extend expander to support avx512
> >         instruction.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         PR target/98434
> >         * gcc.target/i386/pr98434-1.c: New test.
> >         * gcc.target/i386/pr98434-2.c: New test.
> >         * gcc.target/i386/avx512vl-pr95488-1.c: Adjust testcase.
>
>
>
> --
> BR,
> Hongtao