[PATCH] Don't combine param and return value copies

Segher Boessenkool segher@kernel.crashing.org
Sat May 23 19:40:00 GMT 2015


Hi Alan,

On Sat, May 23, 2015 at 06:03:05PM +0930, Alan Modra wrote:
> This stops combine messing with parameter and return value copies
> from/to hard registers.  Bootstrapped and regression tested
> powerpc64le-linux, powerpc64-linux and x86_64-linux.  In looking at a
> number of different powerpc64le gcc/*.o files, I noticed a few code
> generation improvements.  There were cases where a register copy was
> no longer needed, cmp used in place of mr., and rlwinm instead of
> rldicl.  x86_64 gcc/*.o showed no changes (apart from combine.o of
> course), but should see a small improvement in compile time with this
> change.

Thanks.  Did you see improvements around return as well, or mostly /
only related to the function start?

> The "clear next_use when !can_combine_p" change is to fix a non-bug.
> Given
> 
> 1) insn defining rn
>    ...
> 2) insn defining rn but !can_combine_p
>    ...
> 3) insn using rn
> 
> then create_log_links might create a link from (3) to (1), I thought.
> However, can_combine_p doesn't currently allow this to happen.
> Obviously, any can_combine_p result depending on regno shouldn't give
> a different result at (1) and (2), but there is also at test of
> DF_REF_PRE_POST_MODIFY that can.  The saving grace is that pre/post
> modify insns also use the register, which means next_use[rn] will
> point at (2), not (3), when (1) is processed.
> 
> I came across this because at one stage I considered modifying
> can_combine_p.  Someone who does so in the future might trigger the
> above problem, so I thought it worth posting the change.

I agree it looks like a bug waiting to happen.  Please post it as a
separate patch though.

> +/* Twiddle BLOCK_FOR_INSN to TO for instructions in the first block BB
> +   that we don't want to combine with other instructions.  */
> +
> +static void
> +twiddle_first_block (basic_block bb, basic_block to)

I don't like this much.  Messing with global state makes things harder
to change later.  If it is hard to come up with a good name for a
factor, it usually means it is not such a good factor.

You can do these inside can_combine_{def,use}_p as far as I can see?
Probably need to give those an extra parameter then: for _def, a bool
that says "don't allow moves from hard regs", set when the scan has
encountered a NOTE_INSN_FUNCTION_BEG in this bb; and for _use, a regset
of those hard regs we don't want to allow moves to (those seen in USEs
later in that same block).  Or do it in the main loop itself, _{def,use}
are each called in one place only; whatever works best.

What makes return values different from CALL args here, btw?  Why do
you not want to combine return values, but you leave alone call args?

> +/* Fill in log links field for all insns that we wish to combine.  */

Please don't change this comment; it was more correct before.

> @@ -1103,7 +1192,7 @@ create_log_links (void)
>  		 we might wind up changing the semantics of the insn,
>  		 even if reload can make what appear to be valid
>  		 assignments later.  */
> -	      if (regno < FIRST_PSEUDO_REGISTER
> +	      if (HARD_REGISTER_NUM_P (regno)
>  		  && asm_noperands (PATTERN (use_insn)) >= 0)
>  		continue;

An independent change.


Segher



More information about the Gcc-patches mailing list