RE: [PATCH] arm: Fix up -mcpu=iwmmxt ICEs [PR98849]

[Kyrylo Tkachov via Gcc-patches]

> -----Original Message-----
> From: Jakub Jelinek <ja...@redhat.com>
> Sent: 29 January 2021 09:29
> To: Richard Earnshaw <richard.earns...@arm.com>; Ramana
> Radhakrishnan <ramana.radhakrish...@arm.com>; Kyrylo Tkachov
> <kyrylo.tkac...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; Christophe Lyon <christophe.l...@linaro.org>
> Subject: [PATCH] arm: Fix up -mcpu=iwmmxt ICEs [PR98849]
> 
> Hi!
> 
> The
> https://gcc.gnu.org/r11-6707-
> g7432f255b70811dafaf325d94036ac580891de69
> https://gcc.gnu.org/r11-6708-
> gbfab355012ca0f5219da8beb04f2fdaf757d34b7
> changes moved the vashl/vashr/vlshr expanders from neon.md to vec-
> common.md
> and changed their condition from TARGET_NEON to
> ARM_HAVE_<MODE>_ARITH,
> so that they apply also for TARGET_HAVE_MVE.  But, the
> ARM_HAVE_<MODE>_ARITH
> macros are sometimes true also for TARGET_REALLY_IWMMXT, which at
> least
> from quick skimming of former iwmmxt*.md doesn't have such instructions,
> so it seems incorrect to enable them for iwmmxt.  Furthermore, even if it
> had them, iwmmxt doesn't support any way to broadcast values in those
> modes (vec_duplicate and vec_init optabs) and the middle end relies on
> if the vector x vector shift/rotate patterns are supported it can emit
> vector x scalar shift/rotate by broadcasting the shift amount to a vector.
> 
> As the TARGET_NEON vs. TARGET_REALLY_IWMMXT vs.
> TARGET_HAVE_MVE never seem
> to be enabled together, I think we can just write it the following way.
> 

Yeah, they shouldn't be enabled together.

> Bootstrapped/regtested on armv7hl-linux, ok for trunk?

Ok.
Thanks for fixing this.
Kyrill

> 
> Note, seems iwmmxt actually does support vector x scalar shifts, but doesn't
> really enable the optabs that would tell the middle-end code that it does
> (and neon and mve don't seem to support those).  I'll defer that to anybody
> that cares about iwmmxt (if any).
> 
> 2021-01-29  Jakub Jelinek  <ja...@redhat.com>
> 
>       PR target/98849
>       * config/arm/vec-common.md (mve_vshlq_<supf><mode>,
>       vashl<mode>3, vashr<mode>3, vlshr<mode>3): Add
>       && !TARGET_REALLY_IWMMXT to conditions.
> 
>       * gcc.c-torture/compile/pr98849.c: New test.
> 
> --- gcc/config/arm/vec-common.md.jj   2021-01-25 10:02:28.375126904
> +0100
> +++ gcc/config/arm/vec-common.md      2021-01-28 16:49:30.756916669
> +0100
> @@ -301,7 +301,7 @@ (define_insn "mve_vshlq_<supf><mode>"
>       (unspec:VDQIW [(match_operand:VDQIW 1 "s_register_operand"
> "w,w")
>                      (match_operand:VDQIW 2 "imm_lshift_or_reg_neon"
> "w,Dm")]
>        VSHLQ))]
> -  "ARM_HAVE_<MODE>_ARITH"
> +  "ARM_HAVE_<MODE>_ARITH && !TARGET_REALLY_IWMMXT"
>    "@
>     vshl.<supf>%#<V_sz_elem>\t%<V_reg>0, %<V_reg>1, %<V_reg>2
>     * return neon_output_shift_immediate (\"vshl\", 'i', &operands[2],
> <MODE>mode, VALID_NEON_QREG_MODE (<MODE>mode), true);"
> @@ -312,7 +312,7 @@ (define_expand "vashl<mode>3"
>    [(set (match_operand:VDQIW 0 "s_register_operand" "")
>       (ashift:VDQIW (match_operand:VDQIW 1 "s_register_operand" "")
>                     (match_operand:VDQIW 2 "imm_lshift_or_reg_neon"
> "")))]
> -  "ARM_HAVE_<MODE>_ARITH"
> +  "ARM_HAVE_<MODE>_ARITH && !TARGET_REALLY_IWMMXT"
>  {
>    emit_insn (gen_mve_vshlq_u<mode> (operands[0], operands[1],
> operands[2]));
>    DONE;
> @@ -325,7 +325,7 @@ (define_expand "vashr<mode>3"
>    [(set (match_operand:VDQIW 0 "s_register_operand")
>       (ashiftrt:VDQIW (match_operand:VDQIW 1 "s_register_operand")
>                       (match_operand:VDQIW 2
> "imm_rshift_or_reg_neon")))]
> -  "ARM_HAVE_<MODE>_ARITH"
> +  "ARM_HAVE_<MODE>_ARITH && !TARGET_REALLY_IWMMXT"
>  {
>    if (s_register_operand (operands[2], <MODE>mode))
>      {
> @@ -343,7 +343,7 @@ (define_expand "vlshr<mode>3"
>    [(set (match_operand:VDQIW 0 "s_register_operand")
>       (lshiftrt:VDQIW (match_operand:VDQIW 1 "s_register_operand")
>                       (match_operand:VDQIW 2
> "imm_rshift_or_reg_neon")))]
> -  "ARM_HAVE_<MODE>_ARITH"
> +  "ARM_HAVE_<MODE>_ARITH && !TARGET_REALLY_IWMMXT"
>  {
>    if (s_register_operand (operands[2], <MODE>mode))
>      {
> --- gcc/testsuite/gcc.c-torture/compile/pr98849.c.jj  2021-01-28
> 16:45:01.168964754 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/pr98849.c     2021-01-28
> 16:44:22.702399683 +0100
> @@ -0,0 +1,60 @@
> +/* PR target/98849 */
> +
> +unsigned int a[1024], b[1024];
> +int c[1024], d[1024];
> +
> +void
> +f1 (void)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    a[i] = b[i] << 3;
> +}
> +
> +void
> +f2 (int x)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    a[i] = b[i] << x;
> +}
> +
> +void
> +f3 (void)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    c[i] = d[i] << 3;
> +}
> +
> +void
> +f4 (int x)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    c[i] = d[i] << x;
> +}
> +
> +void
> +f5 (void)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    a[i] = b[i] >> 3;
> +}
> +
> +void
> +f6 (int x)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    a[i] = b[i] >> x;
> +}
> +
> +void
> +f7 (void)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    c[i] = d[i] >> 3;
> +}
> +
> +void
> +f8 (int x)
> +{
> +  for (int i = 0; i < 1024; i++)
> +    c[i] = d[i] >> x;
> +}
> 
>       Jakub