[Bug target/98849] New: [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[Bug target/98849] New: [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

            Bug ID: 98849
           Summary: [11 Regression] ICE in expand_shift_1, at
                    expmed.c:2658 since g:7432f255b70811dafaf325d9403
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: marxin at gcc dot gnu.org
                CC: clyon at gcc dot gnu.org
  Target Milestone: ---
              Host: x86_64-linux
            Target: arm-linux-gnueabi

Since the revision the following fails:

$ cat shift.c
typedef int V __attribute__((vector_size(4 * sizeof(int))));
V foo_x, foo_y;
V foo() { return foo_x << foo_y[0]; }

$ arm-linux-gnueabi-gcc shift.c -mcpu=iwmmxt -c
during RTL pass: expand
shift.c: In function ‘foo’:
shift.c:3:24: internal compiler error: in expand_shift_1, at expmed.c:2658
    3 | V foo() { return foo_x << foo_y[0]; }
      |                  ~~~~~~^~~~~~~~~~~
0x5ee097 expand_shift_1
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/expmed.c:2658
0x8360d5 expand_variable_shift(tree_code, machine_mode, rtx_def*, tree_node*,
rtx_def*, int)
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/expmed.c:2699
0x8452b1 expand_expr_real_2(separate_ops*, rtx_def*, machine_mode,
expand_modifier)
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/expr.c:9631
0x74b3e5 expand_gimple_stmt_1
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/cfgexpand.c:3946
0x74b3e5 expand_gimple_stmt
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/cfgexpand.c:4007
0x7510ea expand_gimple_basic_block
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/cfgexpand.c:6044
0x7510ea execute
       
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-trunk-arm/build/gcc/cfgexpand.c:6728
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
      Known to fail|                            |11.0
   Target Milestone|---                         |11.0
      Known to work|                            |10.2.0
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-01-27

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
tree-vect-generic.c claims that
          /* The rtl expander will expand vector/scalar as vector/vector
             if necessary.  Pick one with wider vector type.  */
but that doesn't seem to be the case (at least on this testcase and this
target).
I only see the vashl* used if:
  /* Determine whether the shift/rotate amount is a vector, or scalar.  If the
     shift amount is a vector, use the vector/vector shift patterns.  */
  if (VECTOR_MODE_P (mode) && VECTOR_MODE_P (op1_mode))
    {
      lshift_optab = vashl_optab;
      rshift_arith_optab = vashr_optab;
      rshift_uns_optab = vlshr_optab;
      lrotate_optab = vrotl_optab;
      rrotate_optab = vrotr_optab;
    }    

So, shall tree-vect-generic.c stop relying on that and broadcast the scalar
shift to vector if vector x vector but not vector x scalar shift/rotate is
supported?
Or shall expmed.c do the broadcast?
Or are targets required to support the vector x scalar shift/rotate optabs if
they do support vector x vector?

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
int a[1024], b[1024];

void
foo (void)
{
  for (int i = 0; i < 1024; i++)
    a[i] = b[i] << 3;
}

void
bar (int x)
{
  for (int i = 0; i < 1024; i++)
    a[i] = b[i] << x;
}

ICEs with -O3 -mcpu=iwmmxt too.  Here the vectorizer understands the target has
vector x vector shift and not vector x scalar, so we get:
  vect_cst__13 = { 3, 3 };
...
  vect__2.7_14 = vect__1.6_8 << vect_cst__13;
but veclower carelessly undoes that:
  vect__2.7_14 = vect__1.6_8 << 3;

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For #c2 I've tried:
--- gcc/tree-vect-generic.c.jj  2021-01-04 10:25:38.289239984 +0100
+++ gcc/tree-vect-generic.c     2021-01-27 13:53:28.457752505 +0100
@@ -2147,16 +2147,21 @@ expand_vector_operations_1 (gimple_stmt_
       || code == LROTATE_EXPR
       || code == RROTATE_EXPR)
     {
-      optab opv;
+      optab opv = optab_for_tree_code (code, type, optab_vector);

       /* Check whether we have vector <op> {x,x,x,x} where x
          could be a scalar variable or a constant.  Transform
-         vector <op> {x,x,x,x} ==> vector <op> scalar.  */
+         vector <op> {x,x,x,x} ==> vector <op> scalar, unless
+        the backend only supports vector <op> by vector and not
+        vecot <op> by scalar.  */
       if (VECTOR_INTEGER_TYPE_P (TREE_TYPE (rhs2)))
         {
           tree first;

-          if ((first = ssa_uniform_vector_p (rhs2)) != NULL_TREE)
+          op = optab_for_tree_code (code, type, optab_scalar);
+          if ((first = ssa_uniform_vector_p (rhs2)) != NULL_TREE
+             && (get_compute_type (code, opv, type) != type
+                 || get_compute_type (code, op, type) == type))
             {
               gimple_assign_set_rhs2 (stmt, first);
               update_stmt (stmt);
@@ -2164,7 +2169,6 @@ expand_vector_operations_1 (gimple_stmt_
             }
         }

-      opv = optab_for_tree_code (code, type, optab_vector);
       if (VECTOR_INTEGER_TYPE_P (TREE_TYPE (rhs2)))
        op = opv;
       else
but that doesn't really help, because while veclower21 doesn't undo what the
vectorizer carefully did, match.pd during fre5 breaks it again:
 /* Prefer vector1 << scalar to vector1 << vector2
    if vector2 is uniform.  */
 (for vec (VECTOR_CST CONSTRUCTOR)
  (simplify
   (shiftrotate @0 vec@1)
   (with { tree tem = uniform_vector_p (@1); }
    (if (tem)
     (shiftrotate @0 { tem; }))))))

So, does ARM really only have vector shifts and not scalar?
Though, PowerPC seems to have that too, I'll check out what it does on these
testcases.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ktkachov at gcc dot gnu.org,
                   |                            |rearnsha at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, so on powerpc64le this works fine, expand_binop has:
1558      /* If this is a vector shift by a scalar, see if we can do a vector
1559         shift by a vector.  If so, broadcast the scalar into a vector.  */
1560      if (mclass == MODE_VECTOR_INT)
1561        {
1562          optab otheroptab = unknown_optab;
1563    
1564          if (binoptab == ashl_optab)
1565            otheroptab = vashl_optab;
1566          else if (binoptab == ashr_optab)
1567            otheroptab = vashr_optab;
1568          else if (binoptab == lshr_optab)
1569            otheroptab = vlshr_optab;
1570          else if (binoptab == rotl_optab)
1571            otheroptab = vrotl_optab;
1572          else if (binoptab == rotr_optab)
1573            otheroptab = vrotr_optab;
1574    
1575          if (otheroptab
1576              && (icode = optab_handler (otheroptab, mode)) !=
CODE_FOR_nothing)
1577            {
1578              /* The scalar may have been extended to be too wide. 
Truncate
1579                 it back to the proper size to fit in the broadcast vector.
 */
1580              scalar_mode inner_mode = GET_MODE_INNER (mode);
1581              if (!CONST_INT_P (op1)
1582                  && (GET_MODE_BITSIZE (as_a <scalar_int_mode> (GET_MODE
(op1)))
1583                      > GET_MODE_BITSIZE (inner_mode)))
1584                op1 = force_reg (inner_mode,
1585                                 simplify_gen_unary (TRUNCATE, inner_mode,
op1,
1586                                                     GET_MODE (op1)));
1587              rtx vop1 = expand_vector_broadcast (mode, op1);
1588              if (vop1)
1589                {
1590                  temp = expand_binop_directly (icode, mode, otheroptab,
op0, vop1,
1591                                                target, unsignedp, methods,
last);
1592                  if (temp)
(gdb) 
1593                    return temp;
1594                }
1595            }
1596        }
code for this.  It doesn't work in the ARM case, because it doesn't support
either vec_duplicate_optab nor vec_init_optab for the mode.

I'm declaring this a backend bug, it shouldn't advertise such vector shifts in
configurations in which it can't even init such vectors.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #5 from ktkachov at gcc dot gnu.org ---
Looks like after the refactoring to introduce MVE shifts (which doesn't ICE) we
need to make sure the optab is still disabled for iwmmxt?

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #6 from Christophe Lyon <clyon at gcc dot gnu.org> ---
I'm not familiar with iwmmxt, but the testcase in comment #2 is vectorized
with:
* -mcpu=cortex-a9 -mfpu=auto -mfloat-abi=hard (uses Neon FPU)
* -mcpu=cortex-m55 -mfpu=auto -mfloat-abi=hard (uses MVE/Helium FPU)
in both cases -mfloat-abi=hard is required.

Using -mcpu=iwmmxt -mfpu=auto -mfloat-abi=hard fails because:
cc1: error: '-mfloat-abi=hard': selected processor lacks an FPU

so to answer your question arm does have vector shift by scalar.

But the Neon/MVE patterns use a const_vector constraint (see
mve_vshlq_<supf><mode> and vashl<mode>3 in vec-common.md and ashl<mode>3_iwmmxt
in iwmmxt.md)

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #7 from Christophe Lyon <clyon at gcc dot gnu.org> ---
(In reply to ktkachov from comment #5)
> Looks like after the refactoring to introduce MVE shifts (which doesn't ICE)
> we need to make sure the optab is still disabled for iwmmxt?

So that would mean that ARM_HAVE_<MODE>_ARITH shouldn't be defined for iwmmxt
(only for shifts?) ?

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Seems vec_init optab is supported if TARGET_NEON || TARGET_HAVE_MVE, so maybe
guard the shift expander also on && (TARGET_NEON || TARGET_HAVE_MVE)?
Or && !TARGET_REALLY_IWMMXT.  Dunno if one can mix iwmmxt with neon or mve etc.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #6)
> so to answer your question arm does have vector shift by scalar.

If it does, it doesn't advertize them:
make mddump
grep '"v\?ashlv[0-9qhsdi]*3"' tmp-mddump.md 
(define_expand ("vashlv8qi3")
(define_expand ("vashlv16qi3")
(define_expand ("vashlv4hi3")
(define_expand ("vashlv8hi3")
(define_expand ("vashlv2si3")
(define_expand ("vashlv4si3")
Ditto for ashr and lshr instead of ashl.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
./cc1 -quiet -nostdinc -O3 -mcpu=iwmmxt pr98849-2.c -fdump-tree-all-folding
-mfpu=neon
cc1: error: iWMMXt and NEON are incompatible
So I think TARGET_NEON && TARGET_REALLY_IWMMXT is never true.
Don't know if TARGET_HAVE_MVE && TARGET_REALLY_IWMMXT is similarly never true,
but I'd guess so.  So perhaps just add && !TARGET_REALLY_IWMMXT to the two
conditions.  It is also unclear why you call the pattern mve_* when it is used
by both neon and mve.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[clyon at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #11 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Yes MVE is incompatible with iWMMXt.

Regarding the pattern name, quoting what I wrote in the commit message:
I kept the mve_vshlq_<supf><mode> naming instead of renaming it to
ashl3_<supf>_<mode> as discussed because the reference in
arm_mve_builtins.def automatically inserts the "mve_" prefix and I
didn't want to make a special case for this.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50076
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50076&action=edit
gcc11-pr98849.patch

So like this then?  From quick skimming of iwmmxt.md, it does have the vector
by scalar shifts, but doesn't have vector by vector shifts, so it seems correct
to me to do what this patch does, plus if somebody cared about IWMMXT, it could
announce its shifts through the standard pattern names.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e7429bc9d60c0cb9809a8040bb63dbb9390f40f1

commit r11-6970-ge7429bc9d60c0cb9809a8040bb63dbb9390f40f1
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Jan 29 11:54:22 2021 +0100

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

    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.

    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  <jakub@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.

[Bug target/98849] [11 Regression] ICE in expand_shift_1, at expmed.c:2658 since g:7432f255b70811dafaf325d9403

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98849

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.