This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Out-of-order update of new_spill_reg_store[]


Thanks for looking at this.

"Ulrich Weigand" <uweigand@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 original value of SET_SRC before substitution ...

But I think the idea is that any input reload like that will be copying
the current SET_SRC to another register.  And the hope is presumably
that a R1 <- R2 reload won't reuse R2, even if it needs secondary reloads.
Again, I agree it doesn't feel robust though.

> 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. :-)

It looks like the main sticking point for both you and Bernd is the:

		{
		  rtx set = single_set (insn);
		  if (set && SET_DEST (set) == rld[r].out)
		    {
		      int k;

		      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;
			    }
			}
		    }
		}

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.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]