[Bug target/98877] New: [AArch64] Inefficient code generated for tbl NEON intrinsics

[Bug target/98877] New: [AArch64] Inefficient code generated for tbl NEON intrinsics

[spop at gcc dot gnu.org]

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

            Bug ID: 98877
           Summary: [AArch64] Inefficient code generated for tbl NEON
                    intrinsics
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: spop at gcc dot gnu.org
  Target Milestone: ---

The use of NEON intrinsics is inefficient and leads developers to prefer inline
assembly instead of intrinsics.

A similar performance bug for vmlal intrinsics was reported in
https://gcc.gnu.org/PR92665
The code generated by GCC for table lookups is also inefficient:

$ cat red.c
#include "arm_neon.h"

uint8x16_t fun(uint8x16_t lo, uint8x16_t hi, uint8x16_t idx) {
  uint8x16x2_t tab = { .val = {lo, hi} };
  uint8x16_t res = vqtbl2q_u8(tab, idx);
  return res;
}

$ gcc -O3 -S -o- red.c
fun:
        mov     v4.16b, v0.16b
        mov     v5.16b, v1.16b
        tbl     v0.16b, {v4.16b - v5.16b}, v2.16b
        ret

$ clang -O3 -S -o- red.c
fun:
        tbl     v0.16b, { v0.16b, v1.16b }, v2.16b
        ret

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am 90% sure this is just a register allocation issue.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |91753

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
See PR 91753 for another example.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91753
[Bug 91753] Bad register allocation of multi-register types

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
                 CC|                            |ktkachov at gcc dot gnu.org,
                   |                            |tnfchris at gcc dot gnu.org
   Last reconfirmed|                            |2021-01-29
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from ktkachov at gcc dot gnu.org ---
Confirmed. I think the whole moving in and out the structure modes (OImode,
XImode and friends) really hurts codegen at the RTL level.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[tnfchris at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98877
Bug 98877 depends on bug 91753, which changed state.

Bug 91753 Summary: Bad register allocation of multi-register types
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91753

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

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Here is another example where GCC messes up:

#include "arm_neon.h"
uint8x16_t g(void);
uint8x16_t fun(uint8x16_t lo, uint8x16_t hi, uint8x16_t idx) {
  uint8x16x2_t tab = { .val = {g(), g()} };
  uint8x16_t res = vqtbl2q_u8(tab, idx);
  return res;
}

Note clang/LLVM messes the above one up even worse.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[tnfchris at gcc dot gnu.org]

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

Tamar Christina <tnfchris at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|11.0                        |12.0

--- Comment #5 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
We're in the process of rewriting these intrinsics, should be fixed in GCC 12.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
In the original testcase, there are still extra movs.

For the testcase in comment #4, it is fixed on the trunk and we now get:
```
fun:
        stp     x29, x30, [sp, -48]!
        mov     x29, sp
        str     q2, [sp, 32]
        bl      g
        str     q0, [sp, 16]
        bl      g
        ldp     q30, q2, [sp, 16]
        mov     v31.16b, v0.16b
        ldp     x29, x30, [sp], 48
        tbl     v0.16b, {v30.16b - v31.16b}, v2.16b
        ret
```

Maybe the issue is only with arguments now.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
>Maybe the issue is only with arguments now.


Actually I think this is still a subreg vs ra issue.


(insn 8 5 9 2 (set (subreg:V16QI (reg/v:V2x16QI 100 [ __tab ]) 0)
        (reg/v:V16QI 102 [ lo ])) -1
     (nil))
(insn 9 8 10 2 (set (subreg:V16QI (reg/v:V2x16QI 100 [ __tab ]) 16)
        (reg/v:V16QI 103 [ hi ])) -1
     (nil))
(insn 10 9 11 2 (set (reg:V16QI 101 [ <retval> ])
        (unspec:V16QI [
                (reg/v:V2x16QI 100 [ __tab ])
                (reg/v:V16QI 104 [ idx ])
            ] UNSPEC_TBL))
"/opt/compiler-explorer/arm64/gcc-trunk-20240227/aarch64-unknown-linux-gnu/lib/gcc/aarch64-unknown-linux-gnu/14.0.1/include/arm_neon.h":19566:43
-1
     (nil))

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[rsandifo at gcc dot gnu.org]

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

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

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

--- Comment #8 from Richard Sandiford <rsandifo at gcc dot gnu.org> ---
The reason early_ra doesn't help with the original testcase is that early_ra
punts on any non-move instruction that has a hard register destination.  And it
does that because it can't cope well with cases where hard-coded destinations
force the wrong choice (unlike the proper allocators, which can change the
destination where necessary).  The restriction is needed to avoid regressing
SVE ACLE tests.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[tnfchris at gcc dot gnu.org]

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

--- Comment #9 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
While RA should be able to deal with this,
shouldn't we also just lower TBLs in gimple?

This no reason why this can't be a VEC_PERM_EXPR which would also get the
copies
removed at the gimple level and allows us to optimize this to something else
depending on the index.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[pinskia at gcc dot gnu.org]

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

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Tamar Christina from comment #9)
> While RA should be able to deal with this,
> shouldn't we also just lower TBLs in gimple?
> 
> This no reason why this can't be a VEC_PERM_EXPR which would also get the
> copies
> removed at the gimple level and allows us to optimize this to something else
> depending on the index.

Yes there is a reason, `out of range` values for VEC_PERM is undefined while
tbl is well defined  ( If an index is out of range for the table, the result
for that lookup is 0.).

For tbx, it is well defined also (If an index is out of range for the table,
the existing value in the vector element of the destination register is left
unchanged. ).

I think for VECTOR_CST we can fold it down to VEC_PERM_EXPR if there is no out
of bounds value though.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[tnfchris at gcc dot gnu.org]

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

--- Comment #11 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #10)
> (In reply to Tamar Christina from comment #9)
> > While RA should be able to deal with this,
> > shouldn't we also just lower TBLs in gimple?
> > 
> > This no reason why this can't be a VEC_PERM_EXPR which would also get the
> > copies
> > removed at the gimple level and allows us to optimize this to something else
> > depending on the index.
> 
> Yes there is a reason, `out of range` values for VEC_PERM is undefined while
> tbl is well defined  ( If an index is out of range for the table, the result
> for that lookup is 0.).
> 

I don't think that's not a good reason. The out of range values can be made
implementation defined. i.e. mid-end shouldn't care about them.

not lowering this in gimple means we lose a heck of a lot of optimizations that
are impossible to cover in RTL.

[Bug target/98877] [AArch64] Inefficient code generated for tbl NEON intrinsics

[tnfchris at gcc dot gnu.org]

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

--- Comment #12 from Tamar Christina <tnfchris at gcc dot gnu.org> ---
and it's not the first time we have conditional lowering. We already do so for
e.g. shifts, where shifting by an amount => bitsize of a vector element is
defined behavior or AArch64.