[Bug target/96528] New: [11 Regression] vector comparisons on ARM

[Bug target/96528] New: [11 Regression] vector comparisons on ARM

[glisse at gcc dot gnu.org]

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

            Bug ID: 96528
           Summary: [11 Regression] vector comparisons on ARM
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: enhancement
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glisse at gcc dot gnu.org
  Target Milestone: ---
            Target: arm-none-linux-gnueabihf

(see the discussion after
https://gcc.gnu.org/pipermail/gcc-patches/2020-August/551468.html )

I am using a compiler configured with --target=arm-none-linux-gnueabihf
--with-float=hard --with-cpu=cortex-a9 --with-fpu=neon-fp16

typedef unsigned int vec __attribute__((vector_size(16)));
typedef int vi __attribute__((vector_size(16)));
vi f(vec a,vec b){
    return a==5 | b==7;
}

Compiling with -O yields very long scalar code. Adding -fno-tree-forwprop gets
back the nice, vector code. (at higher optimization levels, one may also need
to disable vrp)

This is due to the fact that while the ARM target handles VEC_COND_EXPR<v == w,
-1, 0> just fine, it does not handle a plain v == w that is not fed directly to
a VEC_COND_EXPR. I was surprised to notice that "grep vec_cmp" gives a number
of lines in the aarch64/ directory, but none in arm/, while AFAIK those neon
instructions are the same. Would it be possible to implement this on ARM as
well? Other middle-end options are also possible, but the difference with
aarch64 makes it tempting to handle it in the target.

[Bug target/96528] [11 Regression] vector comparisons on ARM

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-08-25
   Target Milestone|---                         |11.0
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think that eventually vector lowering should lower

 _1 = a == 5;
 _2 = b == 7;
 _3 = _1 | _2;
 _4 = _3 ? -1 : 0;

to

 _31 = _1 ? -1 : 0;
 _32 = _2 ? -1 : 0;
 _33 = _31 | _32;
 _34 = _33 == -1;
 _4 = _34 ? -1 : 0;

or so.  Conditionals are really a mess, GIMPLE and optabs do not match 1:1
(we've rejected the idea of "splitting" VEC_COND_EXPR to
VEC_COND_{EQ,NE,...}_EXPR and making it four-operand).  Forcing
vector compares to produce a bool vector result was the attempt to make
the GIMPLE IL more streamlined, but obviously the above shows that code
generation needs to be "fixed" - in this case it is vector lowering doing
"code generation" (together with the later isel pass).

[Bug target/96528] [11 Regression] vector comparisons on ARM

[glisse at gcc dot gnu.org]

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

--- Comment #2 from Marc Glisse <glisse at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #1)
> I think that eventually vector lowering should lower
> 
>  _1 = a == 5;
>  _2 = b == 7;
>  _3 = _1 | _2;
>  _4 = _3 ? -1 : 0;
> 
> to
> 
>  _31 = _1 ? -1 : 0;
>  _32 = _2 ? -1 : 0;
>  _33 = _31 | _32;
>  _34 = _33 == -1;

  _34 = _33 < 0; // for consistency with what we do elsewhere? But I think isel
already handles that.

>  _4 = _34 ? -1 : 0;
> 
> or so.

... on platforms like ARM. On AVX512, we clearly want to keep the bool vectors.

> Conditionals are really a mess, GIMPLE and optabs do not match 1:1
> (we've rejected the idea of "splitting" VEC_COND_EXPR to
> VEC_COND_{EQ,NE,...}_EXPR and making it four-operand).  Forcing
> vector compares to produce a bool vector result was the attempt to make
> the GIMPLE IL more streamlined, but obviously the above shows that code
> generation needs to be "fixed" - in this case it is vector lowering doing
> "code generation" (together with the later isel pass).

I wonder if doing it during isel (and updating the predicates so they advertise
a==b as supported for vector lowering when a==b?-1:0 is) would be sufficient.
It requires considering that a bool vector and an integer vector of the same
shape are essentially the same thing on platforms that do not have true bool
vectors. This way we would only need to rewrite
  _1 = a == 5;
to
  _1 = a == 5 ? true : false;

[Bug target/96528] [11 Regression] vector comparisons on ARM

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|enhancement                 |normal
                 CC|                            |rearnsha at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Hmm, I configured exactly as in the description but I do not manage to get nice
code even when veclower sees

  _1 = a_5(D) == { 5, 5, 5, 5 };
  _2 = VEC_COND_EXPR <_1, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
  _3 = b_6(D) == { 7, 7, 7, 7 };
  _4 = VEC_COND_EXPR <_3, { -1, -1, -1, -1 }, { 0, 0, 0, 0 }>;
  _7 = _2 | _4;
  return _7;

I guess I'm missing magic compiler options (as usual with arm...).

Besides 'fixing' the arm backend we can either ISEL the compares as
.VCOND, adjust them in veclower to supported VEC_COND_EXPRs
(need to adjust veclower anyway to not generate the awkward code).

I'll note that the arm backend in its vcond* neon patterns seems to
emit two instructions, a mask generating one and a selecting one
where the mask generating one isn't exposed to the middle-end
(that would be vec_cmp* patterns).

So for the specific case of arm neon I agree the best fix would be
to the target.  It might be possible to copy the aarch64 patterns here.

[Bug target/96528] [11 Regression] vector comparisons on ARM

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rsandifo at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rsandifo at gcc dot gnu.org

--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Mine.

[Bug target/96528] [11 Regression] vector comparisons on ARM

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

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

--- Comment #5 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Fixed by r11-3600.