[PATCH v2] rs6000: Support doubleword swaps removal in rot64 load store [PR100085]

Segher Boessenkool segher@kernel.crashing.org
Tue Jun 8 21:07:45 GMT 2021


Hi!

On Tue, Jun 08, 2021 at 09:11:33AM +0800, Xionghu Luo wrote:
> On P8LE, extra rot64+rot64 load or store instructions are generated
> in float128 to vector __int128 conversion.
> 
> This patch teaches pass swaps to also handle such pattens to remove
> extra swap instructions.

> +/* Return 1 iff PAT is a rotate 64 bit expression; else return 0.  */
> +
> +static bool
> +pattern_is_rotate64_p (rtx pat)

You already have a verb in the name, don't use _p please (and preferably
just don't use it at all, "pattern_is_rotate64" is much better than
"pattern_rotate64_p").

> +{
> +  rtx rot = SET_SRC (pat);

So this is assuming PAT is a SINGLE_SET.  Please say that in the
function comment.

/* Return 1 iff PAT (a SINGLE_SET) is a rotate 64 bit expression; else
   return 0.  */

You can do an assert for that as well, but I wouldn't bother.

> @@ -266,6 +280,9 @@ insn_is_load_p (rtx insn)

(I do realise you just copied existing naming, don't worry :-) )

> @@ -392,7 +411,8 @@ quad_aligned_load_p (swap_web_entry *insn_entry, rtx_insn *insn)
>       false.  */
>    rtx body = PATTERN (def_insn);
>    if (GET_CODE (body) != SET
> -      || GET_CODE (SET_SRC (body)) != VEC_SELECT
> +      || !(GET_CODE (SET_SRC (body)) == VEC_SELECT
> +	  || pattern_is_rotate64_p (body))

Broken indentation: the || should align with "pattern...".

> @@ -2223,9 +2246,9 @@ static void
>  recombine_stvx_pattern (rtx_insn *insn, del_info *to_delete)
>  {
>    rtx body = PATTERN (insn);
> -  gcc_assert (GET_CODE (body) == SET
> -	      && MEM_P (SET_DEST (body))
> -	      && GET_CODE (SET_SRC (body)) == VEC_SELECT);
> +  gcc_assert (GET_CODE (body) == SET && MEM_P (SET_DEST (body))
> +	      && (GET_CODE (SET_SRC (body)) == VEC_SELECT
> +		  || pattern_is_rotate64_p (body)));

Please start a new line for every "&&" here.  The way it was was more
readable.

It often is nice to keep things one one line, if it fits on one line.
If it does not, make a new line for every phrase.  This is more readable
because you can then just scan down the line of "&&" and see the start
of every phrase without actually having to read it all.

> diff --git a/gcc/testsuite/gcc.target/powerpc/float128-call.c b/gcc/testsuite/gcc.target/powerpc/float128-call.c
> index 5895416e985..a1f09df8a57 100644
> --- a/gcc/testsuite/gcc.target/powerpc/float128-call.c
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-call.c
> @@ -21,5 +21,5 @@
>  TYPE one (void) { return ONE; }
>  void store (TYPE a, TYPE *p) { *p = a; }
>  
> -/* { dg-final { scan-assembler "lxvd2x 34"  } } */
> -/* { dg-final { scan-assembler "stxvd2x 34" } } */
> +/* { dg-final { scan-assembler "lvx 2"  } } */
> +/* { dg-final { scan-assembler "stvx 2" } } */

Huh.  Is that correct?  Where did the other 32 loads and stores go?  Are
there now other insns generated that you should scan for?

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr100085.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_float128_sw_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8" } */

If you use float128_ok you should use -mfloat128 (or this is very
surprising and is worth an explanation itself :-) )

But, you do not need it, since you use -mcpu=power8 already (which
implicitly sets this).  So just remove that dg-require please.

> +/* { dg-final { scan-assembler-not "xxpermdi" } } */
> +/* { dg-final { scan-assembler-not "stxvd2x" } } */
> +/* { dg-final { scan-assembler-not "lxvd2x" } } */

It is a good habit to use \m and \M in the scans where you can (those
are the same as \< and \> are in some other regexp dialects).  They
aren't absolutely necessary here of course.


Okay for trunk with those fixes.  Thanks!


Segher


More information about the Gcc-patches mailing list