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
