On Wed, Oct 21, 2020 at 03:25:29AM -0500, Xionghu Luo wrote:
> Don't split code from add<mode>3 for SDI to allow a later pass to split.

This is very problematic.

> This allows later logic to hoist out constant load in add instructions.

Later logic should be able to do that any way (I do not say that works
perfectly, mind; it no doubt could be improved).

> In loop, lis+ori could be hoisted out to improve performance compared with
> previous addis+addi (About 15% on typical case), weak point is
> one more register is used and one more instruction is generated.  i.e.:

Yes, better performance on one testcase, and worse code always :-(

> addis 3,3,0x6765
> addi 3,3,0x4321
> 
> =>
> 
> lis 9,0x6765
> ori 9,9,0x4321
> add 3,3,9

This is the typical kind of clumsy code you get if you generate RTL that
matches actual machine instructions too late ("split too late").

So, please make it possible to hoist 2-insn-immediate sequences out of
loops, *without* changing them to fake 1-insn things.

Some comments on the patch:

> +       rs6000_emit_move (tmp, operands[2], <MODE>mode);
> +       emit_insn (gen_add<mode>3 (operands[0], operands[1], tmp));
>         DONE;
>       }
> +      else
> +     {

You don't need an else here: everything in the "if" has done a DONE or
FAIL.  You can just keep the existing code as-is, there is no need to
obfuscate the code :-)

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/add-const.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile { target { lp64 } } } */

{ target lp64 }
(no extra braces needed)

> +/* Ensure the lis,ori are generated, which indicates they have
> +   been hoisted outside of the loop.  */

That is a very fragile test.

> --- a/gcc/testsuite/gcc.target/powerpc/prefix-add.c
> +++ b/gcc/testsuite/gcc.target/powerpc/prefix-add.c
> @@ -2,13 +2,13 @@
>  /* { dg-require-effective-target powerpc_prefixed_addr } */
>  /* { dg-options "-O2 -mdejagnu-cpu=power10" } */
>  
> -/* Test that PADDI is generated to add a large constant.  */
> +/* Test that PLI is generated to add a large constant.  */

Nope, that is a bad idea.  The test tested that we generate good code,
we should keep it that way.


Segher

Reply via email to