Re: PR 35232: Recent regression due to reload inheritance bug

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

Hi Richard,

> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
> > What about cases where the secondary reload register also had
> > its mode adjusted?  Can this actually happen?

> AFAICT, push_secondary_reload always chooses the inmode of the parent
> reload for inputs and the outmode of the parent reload for outputs,
> so I'm not sure under what conditions:
> 
> 	      if (GET_MODE (reloadreg) != mode)
> 		reloadreg = reload_adjust_reg_for_mode (reloadreg, mode);
> 
> would trigger.  (This call is only used for secondary reload moves,
> not target-specific insns, and has been there in one form or another
> since the beginning of the CVS history.)

Yes, I was wondering why this code was there ...

> I agree -- if this is what you're thinking -- that it's far from obvious
> that the original code is correct.  I assume the "not a single SET" part
> exists because a reload_out* pattern might use UNSPEC_VOLATILEs, or some
> other such thing whose semantics cannot be predicted by reload, and we
> do not want to risk deleting it even if the result of the output reload
> is found to be dead.  This should not happen for a normal move, whose
> semantics ought to be just that: a move.  But if that's true, what about
> reload_out* expanders that create a (set ... (unspec_volatile ...))?
> And why do we not store to new_spill_reg_store[REGNO (rl_reg_rtx)] in
> the "else if" you quote above, if the "else" really is correct for
> "s >= 0" single sets?

Agreed.  I think the latest "else" should really be a "else if (s < 0)".

> However, going back to the "this isn't worse or better after my patch",
> I think it would be best to deal with this as a separate issue.
> Especially given that I'm still hoping to get this patch into 4.3. ;)

OK.  I agree that your patch doesn't change the behaviour here, so I'm
fine with addressing this separately.


> > I'm wondering whether == compares actually work here (and elsewhere),
> > as reload_adjust_reg_for_mode may allocate fresh rtx's ...
> 
> I wondered about that too, but I don't think "rl->reg_rtx != old" ever
> implied "!rtx_equal_p (rl->reg_rtx, old)" before the patch.  I think
> this is just an "early out" for speed reasons, in which case using
> rtx_equal_p might defeat the object.

It's not just an "early out", it's trying to prevent generating an
identity move instruction if the reload register is already equal to
the target register for some reason.  Using rtx_equal_p seems to be
the correct solution here.

It's probably not a big deal if the identity move *is* generated,
as those will be cleaned up later on anyway.  Still, I don't see why
we *wouldn't* want to use rtx_equal_p here.

Anyway, this is not critical, so if you prefer this to be addressed
separately, that's fine with me.


> > Maybe just memset'ing new_spill_reg_store to zero at the start
> > of emit_reload_insns would be clearer?
> 
> TBH, I'd wondered about that too, but the original authors obviously
> wanted to touch as little of the new_spill_reg_store array as possible.
> I decided in the end to avoid changing peripheral decisions like that
> as part of this patch.  (Again, this was partly a "for 4.3" thing.)
> 
> I suppose the thinking was that it was better not to dirty the whole
> new_spill_reg_store array on targets with large register files when
> only a very small number are actually set or used.

OK, well the loop is fine with me.


> > Why do you call the variable "dest" here and not "src" as above?
> > It is still the (potential) source of a future reload inheritance ...
> 
> If you look at each reload as a black box, it's really just a move, and
> this code is trying to record that move for later.  So I was naming SRC
> and DEST after their position in that move.

The code isn't really trying to record the *move*.  It is trying to 
record that a register holds a particular value, so that this value
can be inherited later on.  At this point we do not care at all if
the reason why the register holds the value is because it was the
source or the destination of the reload.

See also the second if, starting below

      /* If a register gets output-reloaded from a non-spill register,
         that invalidates any previous reloaded copy of it.

where the variable is also called "src_reg" -- and gets its value from
either an output or an input reload.

I understood "source" here to mean "source of a future inheritance".


> >>@@ -7687,17 +7718,18 @@ emit_reload_insns (struct insn_chain *ch
> >> 	 it thinks only about the original insn.  So invalidate it here.
> >> 	 Also do the same thing for RELOAD_OTHER constraints where the
> >> 	 output is discarded.  */
> >>-      if (i < 0 
> >>-	  && ((rld[r].out != 0
> >>-	       && (REG_P (rld[r].out)
> >>-		   || (MEM_P (rld[r].out)
> >>+      else if (i < 0
> >>+	       && ((rld[r].out != 0
> >>+		    && (REG_P (rld[r].out)
> >>+			|| (MEM_P (rld[r].out)
> >>+			    && REG_P (rld[r].out_reg))))
> >
> > Why is the "else" necessary?
> 
> That was part of the (rather misguided?) reassignments of "i" and "nr".
> Changing this to "else" meant that we'd never refer to the original
> "i" after "narrowing" it.  It wouldn't have made any difference in
> practice; I just thought this was clearer.  (I'd argue it's a little
> clearer on its own, but without the additional motivation of the
> in-place narrowing, I'm happy to leave it out.)

I see.  I'd prefer to leave this as is (with the "i" unmodified).


> Does the rest of the message convince you?  If so, I'll prepare
> a version of the patch with new local vars and without the
> s/if/else if/.

OK, thanks!

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com