Richard Sandiford wrote:
> Bernd Schmidt <ber...@codesourcery.com> writes:
> >> gcc/
> >> * reload1.c (reload_regs_reach_end_p): Replace with...
> >> (reload_reg_rtx_reaches_end_p): ...this function.
> >> (new_spill_reg_store): Update commentary.
> >> (emit_input_reload_insns): Don't clear new_spill_reg_store here.
> >> (emit_output_reload_insns): Check reload_reg_rtx_reaches_end_p
> >> before setting new_spill_reg_store.
> >> (emit_reload_insns): Use a separate loop to clear new_spill_reg_store.
> >> Use reload_reg_rtx_reaches_end_p instead of reload_regs_reach_end_p.
> >> Also use reload_reg_rtx_reaches_end_p when recording inheritance
> >> information for non-spill reload registers.
> >
> > Just an update to say that based on our discussion I think the general
> > approach is OK, but I'm still trying to figure out what exactly this
> > piece of code is doing, and whether the changes to it make sense:
Not sure whether Bernd was planning to get back to this, but just a couple
of comments from my side ...
> Either way, the idea is that new_spill_reg_store[R] is only valid
> for reload registers R that reach the end of the reload sequence.
> We really should check that H1 reaches the end before using
> new_spill_reg_store[H1].
I'm a bit confused why it is necessary to check that the register reaches
the end of the reload sequence *both* when *setting* new_spill_reg_store,
and again when *using* the contents of new_spill_reg_store ... Wouldn't
the latter be redundant if we already guarantee the former?
> [A] (but not [B]). I think this is for optional reloads that we
> decided not to make, in cases where the source of the set needs
> no reloads. E.g. the pre-reload instruction might be:
>
> (set (reg P1) (reg H1))
>
> and P1 has an optional output reload that we decided not to make.
> Here we record that H1 holds P1 as a possible inheritance.
> If inheritance causes the P1 <- H1 move to become unnecessary,
> we can delete it as dead.
>
> There doesn't seem to be any kind of "reaches end" check, but I
> suppose the hope is that instructions like this are going to be so
> simple that there's no point. Although I'm not sure whether for:
>
> (parallel [(set (reg P1) (reg H1))
> (clobber (scratch))])
>
> we'd ever consider using H1 for the scratch in cases where the
> clobber isn't an earlyclobber.
Well, there could still be an output reload on the instruction, I guess
(pre/post modify of an address register in the SET_DEST ?), which might
even involve secondary reloads ... and if H1 is dead in the insn, it
could possibly be chosen as the reload reg for one of these.
In any case, it seems strange to use some random SET_SRC register for
spill_reg_store purposes. I'd have expected this array to only track
spill registers. In fact, the comment says:
/* If this is an optional reload, try to find the source reg
from an input reload. */
So at least the comment seems to indicate that only case [B] was ever
intended to be handled here, not case [A].
> [B] I think this is similar to [A], but for cases where the source
> is reloaded. E.g. the pre-reload instruction might be:
>
> (set (reg P1) (reg P2))
>
> where P1 has an optional reload that we decided not to make and
> P2 is reloaded into H2. The final sequence would look like:
>
> (set (reg H2) (reg P2))
> (set (reg P1) (reg H2))
>
> and the code would record P1 <- H2 for inheritance.
>
> There does seem to be a real danger here that H2 could be reused
> for clobbers (except again for earlyclobbers), so it seemed best
> to test reload_reg_rtx_reaches_end_p. However, this change was
> by inspection, and isn't directly related to the new handling
> of new_spill_reg_store[], since the inherited "reload" is
> actually the reloaded instruction itself.
>
> If H2 doesn't reach the end, the patch falls back to [A].
> This means that if the original source is a hard register
> of the wrong class (instead of a pseudo as in the example above),
> we can continue to record an inheritance for that.
This has again the problem whether we even want/can handle case [A]
correctly. And even if so, there's the additional problem that if
there is an input reload, the SET_SRC is most likely going to be
replaced by some reload register (note that emit_reload_insns is
called *before* subst_reloads), which makes it really unclear how
valid it is to use the original value of SET_SRC before substitution ...
Maybe the conservative fix would be to not handle case [A], and neither
case [B] when the register does not reach the end. Not sure if this
would show up as performance regression somewhere, but it seems unlikely
to me.
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
ulrich.weig...@de.ibm.com
