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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Richard Sandiford wrote:
> "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.

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.Weigand@de.ibm.com