Re: Out-of-order update of new_spill_reg_store[]
Richard Sandiford wrote:
Ulrich Weigand uweig...@de.ibm.com writes:
Richard Sandiford wrote:
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?
The idea was to handle the case where we use the same register for
two reloads. One of them reaches the end but the other one doesn't.
Ah, now I get it. Thanks for the explanation!
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].
In that case though, I don't really see what the src_reg assignment:
if (set SET_DEST (set) == rld[r].out)
{
int k;
src_reg = SET_SRC (set);
is doing, since it is only used if we can't find an input reload.
Oh, I agree -- I was just pointing out that the code doesn't match the
comment here. I'd assume the intent was to catch even more options for
reload inheritance; I'm just not really sure doing it this way is safe.
(I'm also not sure how much this actually buys us; it would be interesting
to experiment with disabling this code path and see what if any differences
in code generation on various platforms result ...)
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.
I'd hope so too, but since that first src_reg was presumably added for a
reason, I'm a bit reluctant to do something so daring at this stage. :-)
Agreed, this looks like something for later.
block. Since that part wasn't really directly related to the bug
I was trying to fix, more of a misguided attempt to be complete,
my preference would be to go for the second patch, which leaves
the block untouched.
Makes sense to me.
I'd still like to give Bernd a chance to weigh in (since he already looked
at the patch before), but if he doesn't within the next couple of days,
your (second) patch is OK for mainline.
Thanks,
Ulrich
--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
ulrich.weig...@de.ibm.com
Ping^5: Out-of-order update of new_spill_reg_store[]
Ping for this reload inheritance patch: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00266.html or perhaps the original: http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00663.html It fixes some wrong-code regressions in execute/scal-to-vec1.c on mips64-linux-gnu and mips64-elf. Richard
Re: Out-of-order update of new_spill_reg_store[]
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
Re: Out-of-order update of new_spill_reg_store[]
Thanks for looking at this.
Ulrich Weigand uweig...@de.ibm.com writes:
Richard Sandiford wrote:
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?
The idea was to handle the case where we use the same register for
two reloads. One of them reaches the end but the other one doesn't.
(In the testcase, the one that reaches the end is from the second
copy in a secondary output reload, while the first is a normal
copy-out reload.)
We need to check when setting because we set new_spill_reg_store[]
in rld[] order, which isn't necessarily the same as the order in
which the reloads are actually emitted into the insn stream.
So the entry for the wrong reload can end up overwriting the
right one.
We also need to check when using new_spill_reg_store[] because we
consider recording inheritance information for both reloads. Only one
of them actually reaches the end, and new_spill_reg_store[] is only valid
for that one. We shouldn't record any inheritance information for the other.
[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.
Hopefully that particular case wouldn't be a problem because autoinc
reloads need a register that is free both before and after the instruction.
But I agree this code doesn't look particularly robust.
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].
In that case though, I don't really see what the src_reg assignment:
if (set SET_DEST (set) == rld[r].out)
{
int k;
src_reg = SET_SRC (set);
is doing, since it is only used if we can't find an input reload.
[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
Ping^4: Out-of-order update of new_spill_reg_store[]
Ping for this reload patch: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00266.html or perhaps the original: http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00663.html which fixes some wrong-code regressions on mips64-linux-gnu. Richard
Ping^3: Out-of-order update of new_spill_reg_store[]
Ping for this reload patch: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00266.html or perhaps the original: http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00663.html which fixes some wrong-code regressions on mips64-linux-gnu. Richard
Ping^2: Out-of-order update of new_spill_reg_store[]
Ping for this reload patch: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00266.html or perhaps the original: http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00663.html which fixes some wrong-code regressions on mips64-linux-gnu. Richard
Ping: Out-of-order update of new_spill_reg_store[]
Ping for this reload patch: http://gcc.gnu.org/ml/gcc-patches/2011-12/msg00266.html or perhaps the original: http://gcc.gnu.org/ml/gcc-patches/2011-10/msg00663.html which fixes some wrong-code regressions on mips64-linux-gnu. Richard
Re: Out-of-order update of new_spill_reg_store[]
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:
@@ -8329,30 +8329,33 @@ emit_reload_insns (struct insn_chain *ch
the storing insn so that we may delete this insn with
delete_output_reload. */
src_reg = reload_reg_rtx_for_output[r];
-
- /* If this is an optional reload, try to find the source reg
- from an input reload. */
- if (! src_reg)
+ if (src_reg
+reload_reg_rtx_reaches_end_p (src_reg, r))
+ store_insn = new_spill_reg_store[REGNO (src_reg)];
+ else
{
+ /* If this is an optional reload, try to find the
+ source reg from an input reload. */
rtx set = single_set (insn);
if (set SET_DEST (set) == rld[r].out)
{
int k;
+ rtx cand;
src_reg = SET_SRC (set);
store_insn = insn;
for (k = 0; k n_reloads; k++)
- {
- if (rld[k].in == src_reg)
- {
- src_reg = reload_reg_rtx_for_input[k];
- break;
- }
- }
+ if (rld[k].in == src_reg)
+ {
+ cand = reload_reg_rtx_for_input[k];
+ if (reload_reg_rtx_reaches_end_p (cand, k))
+ {
+ src_reg = cand;
+ break;
+ }
+ }
}
}
- else
- store_insn = new_spill_reg_store[REGNO (src_reg)];
if (src_reg REG_P (src_reg)
REGNO (src_reg) FIRST_PSEUDO_REGISTER)
{
Bernd
Re: Out-of-order update of new_spill_reg_store[]
On 10/11/11 14:35, Richard Sandiford wrote: No, reload 1 is inherited by a later instruction. And it's inherited correctly, in terms of the register contents being what we expect. (Reload 1 is the one that survives to the end of the instruction's reload sequence. Reload 2, in contrast, is clobbered by reload 1, so could not be inherited. So when we record inheritance information in emit_reload_insns, reload_reg_reaches_end_p correctly stops us from recording reload 2 but allows us to record reload 1.) The problem is that we record the wrong instruction for reload 1. We say that reload 1 is performed by the instruction that performs reload 2. So spill_reg_store[] contains the instruction for reload 2 rather than the instruction for reload 1. We delete it in delete_output_reload at the point of inheritance. Ok. So, would the minimal fix of testing !new_spill_reg_store[..] before writing to it also work? Seems to me this would cope with the out-of-order writes by only allowing the first. If so, then I think I'd prefer that, but we could gcc_assert (reload_reg_reaches_end (..)) as a bit of a verification of that function. Bernd
Re: Out-of-order update of new_spill_reg_store[]
Bernd Schmidt ber...@codesourcery.com writes: On 10/11/11 14:35, Richard Sandiford wrote: No, reload 1 is inherited by a later instruction. And it's inherited correctly, in terms of the register contents being what we expect. (Reload 1 is the one that survives to the end of the instruction's reload sequence. Reload 2, in contrast, is clobbered by reload 1, so could not be inherited. So when we record inheritance information in emit_reload_insns, reload_reg_reaches_end_p correctly stops us from recording reload 2 but allows us to record reload 1.) The problem is that we record the wrong instruction for reload 1. We say that reload 1 is performed by the instruction that performs reload 2. So spill_reg_store[] contains the instruction for reload 2 rather than the instruction for reload 1. We delete it in delete_output_reload at the point of inheritance. Ok. So, would the minimal fix of testing !new_spill_reg_store[..] before writing to it also work? Seems to me this would cope with the out-of-order writes by only allowing the first. If so, then I think I'd prefer that, but we could gcc_assert (reload_reg_reaches_end (..)) as a bit of a verification of that function. I don't think the assert would be safe. We could have similar reuse in cases where the first reload (in rld order) is a double-register value starting in $4 and the second reload uses just $5. In that case, the first reload will have set new_spill_reg_store[4], so new_spill_reg_store[5] will still be null. But $5 in the second reload won't survive until the end of the sequence. So we'd try to set new_spill_reg_store[5] and trip the assert. IMO it's a choice between just checking for null and not asserting (if that really is safe, since we'll be storing instructions that don't actually reach the end of the reload sequence), or checking reload_reg_reaches_end. I prefer the second, since it seems more direct, and matches the corresponding code in emit_reload_insns. Richard
Re: Out-of-order update of new_spill_reg_store[]
I'm not completely following this yet, so please bear with me...
On 10/09/11 10:01, Richard Sandiford wrote:
Reload 0: GR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0), can't combine,
secondary_reload_p
reload_reg_rtx: (reg:SI 5 $5)
Reload 1: reload_out (SI) = (reg:SI 32 $f0 [1655])
MD1_REG, RELOAD_FOR_OUTPUT (opnum = 0)
reload_out_reg: (reg:SI 32 $f0 [1655])
reload_reg_rtx: (reg:SI 65 lo)
secondary_out_reload = 0
Reload 2: reload_out (SI) = (reg:SI 1656)
GR_REGS, RELOAD_FOR_OUTPUT (opnum = 3)
reload_out_reg: (reg:SI 1656)
reload_reg_rtx: (reg:SI 5 $5)
So $5 is first stored in 1656 (operand 3), then $5 is used a secondary
reload in copying LO to $f0 (operand 0, reg 1655). The next and final
use of 1655 ends up inheriting this second reload of $5, so we try to
delete the original output copy. The problem is that we delete the
wrong one: we delete the store of $5 to 1656 rather than the copy of
$5 to 1655/$f0.
So, reload 1 inherited from somewhere else rather than using reg $5 from
its secondary reload? Where do we try to delete the insn, and what's the
state of the spill_reg_store data at that point?
The fix I went for is to clear new_spill_reg_store[] for all reloads
as a separate pass (rather than in the main do_{input,output}_reload
loop), then only allow new_spill_store_reg[] to be set if the associated
reload register reaches the end of the reload sequence.
In this case, reload 0 is emitted after reload 2, so it reaches the end.
Correct? What would happen if the 0/1 pair and 2 were swapped?
Bernd
Out-of-order update of new_spill_reg_store[]
This patch fixes an ordering problem in reload: the output reloads are
emitted in reverse operand order, but new_spill_reg_store[] is updated
in forward reload order. This causes problems if the same register is
used for two reloads.
I saw this hit on mips64-linux-gnu/-mabi=64 as a failure in
execute/scal-to-vec1.c at -O3. The reloads were:
Reloads for insn # 580
Reload 0: GR_REGS, RELOAD_FOR_OUTPUT_ADDRESS (opnum = 0), can't combine,
secondary_reload_p
reload_reg_rtx: (reg:SI 5 $5)
Reload 1: reload_out (SI) = (reg:SI 32 $f0 [1655])
MD1_REG, RELOAD_FOR_OUTPUT (opnum = 0)
reload_out_reg: (reg:SI 32 $f0 [1655])
reload_reg_rtx: (reg:SI 65 lo)
secondary_out_reload = 0
Reload 2: reload_out (SI) = (reg:SI 1656)
GR_REGS, RELOAD_FOR_OUTPUT (opnum = 3)
reload_out_reg: (reg:SI 1656)
reload_reg_rtx: (reg:SI 5 $5)
So $5 is first stored in 1656 (operand 3), then $5 is used a secondary
reload in copying LO to $f0 (operand 0, reg 1655). The next and final
use of 1655 ends up inheriting this second reload of $5, so we try to
delete the original output copy. The problem is that we delete the
wrong one: we delete the store of $5 to 1656 rather than the copy of
$5 to 1655/$f0.
The fix I went for is to clear new_spill_reg_store[] for all reloads
as a separate pass (rather than in the main do_{input,output}_reload
loop), then only allow new_spill_store_reg[] to be set if the associated
reload register reaches the end of the reload sequence.
emit_input_reloads has:
/* Output a special code sequence for this case, and forget about
spill reg information. */
new_spill_reg_store[REGNO (reloadreg)] = NULL;
inc_for_reload (reloadreg, oldequiv, rl-out, rl-inc);
I think this store is redundant: emit_reload_insns should already have
cleared it, beth before and after the patch. (The code was originally:
/* Output a special code sequence for this case. */
new_spill_reg_store[REGNO (reloadreg)]
= inc_for_reload (reloadreg, oldequiv, rl-out,
rl-inc);
but was changed because we can't inherit auto-inc reloads as easily
as that. So the nullification came from an existing new_spill_reg_store[]
assignment, rather than being added explicitly.)
Also, emit_reload_insns has two blocks to record inheritance information:
one for spill registers and one for non-spill registers. The spill version
checks that the reload register reaches the end of the sequence, and I think
the non-spill version should too.
Tested on mips64-linux-gnu and x86_64-linux-gnu. It fixes the testcase
(by deleting the correct instruction -- the inheritance still happens).
Bernd, Uli, does this look OK?
Richard
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.
Index: gcc/reload1.c
===
--- gcc/reload1.c 2011-10-08 16:32:26.0 +0100
+++ gcc/reload1.c 2011-10-08 16:32:26.0 +0100
@@ -5499,15 +5499,15 @@ reload_reg_reaches_end_p (unsigned int r
}
/* Like reload_reg_reaches_end_p, but check that the condition holds for
- every register in the range [REGNO, REGNO + NREGS). */
+ every register in REG. */
static bool
-reload_regs_reach_end_p (unsigned int regno, int nregs, int reloadnum)
+reload_reg_rtx_reaches_end_p (rtx reg, int reloadnum)
{
- int i;
+ unsigned int i;
- for (i = 0; i nregs; i++)
-if (!reload_reg_reaches_end_p (regno + i, reloadnum))
+ for (i = REGNO (reg); i END_REGNO (reg); i++)
+if (!reload_reg_reaches_end_p (i, reloadnum))
return false;
return true;
}
@@ -7052,7 +7052,9 @@ static rtx operand_reload_insns = 0;
static rtx other_operand_reload_insns = 0;
static rtx other_output_reload_insns[MAX_RECOG_OPERANDS];
-/* Values to be put in spill_reg_store are put here first. */
+/* Values to be put in spill_reg_store are put here first. Instructions
+ must only be placed here if the associated reload register reaches
+ the end of the instruction's reload sequence. */
static rtx new_spill_reg_store[FIRST_PSEUDO_REGISTER];
static HARD_REG_SET reg_reloaded_died;
@@ -7213,9 +7215,7 @@ emit_input_reload_insns (struct insn_cha
/* Prevent normal processing of this reload. */
special = 1;
- /* Output a special code sequence for this case, and forget about
-spill
