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
