Re: [PATCH-1, rs6000] Generate permute index directly for little endian target [PR100866]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Haochen,

Sorry for the late review.

on 2022/10/11 15:38, HAO CHEN GUI wrote:
> Hi,
>   This patch modifies the help function which generates permute index for
> vector byte reversion and generates permute index directly for little endian
> targets. It saves one "xxlnor" instructions on P8 little endian targets as
> the original process needs an "xxlnor" to calculate complement for the index.
> 

Nice.

>     Bootstrapped and tested on ppc64 Linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> ChangeLog
> 2022-10-11  Haochen Gui <guihaoc@linux.ibm.com>
> 
> gcc/
> 	PR target/100866
> 	* config/rs6000/rs6000-call.cc (swap_endian_selector_for_mode):
> 	Generate permute index directly for little endian targets.
> 	* config/rs6000/vsx.md (revb_<mode>): Call vprem directly with
> 	corresponding permute indexes.
> 
> gcc/testsuite/
> 	PR target/100866
> 	* gcc.target/powerpc/pr100866.c: New.
> 
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000-call.cc b/gcc/config/rs6000/rs6000-call.cc
> index 551968b0995..bad8e9e0e52 100644
> --- a/gcc/config/rs6000/rs6000-call.cc
> +++ b/gcc/config/rs6000/rs6000-call.cc
> @@ -2839,7 +2839,10 @@ swap_endian_selector_for_mode (machine_mode mode)
>      }
> 
>    for (i = 0; i < 16; ++i)
> -    perm[i] = GEN_INT (swaparray[i]);
> +    if (BYTES_BIG_ENDIAN)
> +      perm[i] = GEN_INT (swaparray[i]);
> +    else
> +      perm[i] = GEN_INT (~swaparray[i] & 0x0000001f);

IMHO, it would be good to add a function comment for this function,
it's sad that we didn't have it before.  With this patch, the selector (perm) is
expected to be used with vperm direct as shown below, it would be good to note it
explicitly for other potential callers too.

> 
>    return force_reg (V16QImode, gen_rtx_CONST_VECTOR (V16QImode,
>  						     gen_rtvec_v (16, perm)));
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index e226a93bbe5..b68eba48d2c 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -6096,8 +6096,8 @@ (define_expand "revb_<mode>"
>  	 to the endian mode in use, i.e. in LE mode, put elements
>  	 in BE order.  */
>        rtx sel = swap_endian_selector_for_mode(<MODE>mode);
> -      emit_insn (gen_altivec_vperm_<mode> (operands[0], operands[1],
> -					   operands[1], sel));
> +      emit_insn (gen_altivec_vperm_<mode>_direct (operands[0], operands[1],
> +						  operands[1], sel));>      }
> 
>    DONE;
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr100866.c b/gcc/testsuite/gcc.target/powerpc/pr100866.c
> new file mode 100644
> index 00000000000..c708dfd502e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr100866.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8" } */
> +/* { dg-final { scan-assembler-not "xxlnor" } } */

Nit: may be better with {\mxxlnor\M}?

The others look good to me.  Thanks!

BR,
Kewen

> +
> +#include <altivec.h>
> +
> +vector unsigned short revb(vector unsigned short a)
> +{
> +   return vec_revb(a);
> +}
>