Re: [PATCH] cprop: Don't replace fixed hard regs with pseudos [PR93124]

[Jeff Law <law at redhat dot com>]

On Thu, 2020-01-23 at 11:43 +0000, Richard Sandiford wrote:
> Jeff Law <law@redhat.com> writes:
> > On Wed, 2020-01-22 at 12:02 +0000, Richard Sandiford wrote:
> > > One consequence of r276318 was that cselib now preserves sp-based
> > > values across function calls.  This in turn convinced cprop to
> > > replace the clobber in:
> > > 
> > >    (set (reg PSUEDO) (reg sp))
> > >    ...
> > >    (call ...)
> > >    ...
> > >    (clobber (mem:BLK (reg sp)))
> > > 
> > > with:
> > > 
> > >    (clobber (mem:BLK (reg PSEUDO)))
> > > 
> > > But I doubt this could ever be an optimisation, regardless of what the
> > > changed instruction is.  Extending the lifetimes of pseudos can lead to
> > > extra spills, whereas sp is available everywhere.
> > > 
> > > More generally, I don't think we should replace any fixed hard register
> > > with a pseudo.  Replacing them with a constant is still potentially
> > > useful though, since we'll only make the change if the insn pattern
> > > allows it.
> > > 
> > > This part 1 of the fix for PR93124.  Part 2 contains the testcase.
> > > 
> > > Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
> > > 
> > > Richard
> > > 
> > > 
> > > 2020-01-22  Richard Sandiford  <richard.sandiford@arm.com>
> > > 
> > > gcc/
> > > 	PR rtl-optimization/93124
> > > 	* cprop.c (cprop_replace_with_reg_p): New function.
> > > 	(cprop_insn, do_local_cprop): Use it.
> > In theory there may be cases where replacing a fixed hard register with
> > a pseudo  in turn might allow a allocation of the pseudo to a different
> > hard register which *could* have a different cost.
> > 
> > But in a CLOBBER insn, none of that should matter.  Would it make sense
> > to only do this on CLOBBERS?  I'm not rejecting.  Mostly I'm worried
> > about unintended consequences and wondering if we narrow the cases
> > where we're changing behavior that unintended consequences are less
> > likely to pop up.
> 
> Yeah, I guess there is a danger of unintended consequences.  E.g. on
> aarch64 the sp register isn't usable everywhere that a normal GPR is.
> Ideally that kind of thing would be enforced by the predicates,
> but it generally isn't yet, and it would be nice if the .md format
> could make this easier to get right (e.g. generating predicates
> automatically from constraints for simple cases).
> 
> I didn't make it clear at all, but clobbers don't really pose a separate
> problem in the context of this patch.  I assume we made this kind of
> sp->pseudo change between call boundaries before r276318 too, and the
> auto-inc-dec patch is enough to fix the PR on its own.
Right.  You may not have been explicit, but I certainly got the
impression that the auto-inc-dec patch was sufficient to fix this
instance, but both provide a higher degree of coverage.

> 
> It's just that when I saw where the (clobber (mem (reg PSEUDO))) was
> coming from, it looked like a misfeature for general non-clobber insns
> too.  In the testcase it was making a pseudo live across a call when it
> wasn't before, meaning that either the pseudo would need to be spilled
> around the call or that we'd need to save&restore an extra call-saved
> register.  The fact that we did this for an sp equivalence is a
> regression from GCC 9.
ACK.  Interestingly enough we had another problem in this space pop up
today.

Finding a way to drop the naked clobbers/uses would be a better way
forward.  I'm a bit surprised we need them as much as we apparently do.
We're conflating issues a bit here though.


> 
> But maybe the patch is tackling this in the wrong place.  In general,
> I think we should be careful about propagating registers across calls
> that they were previously dead at, and this should probably be tackled
> in those terms instead.
Yea, there's certainly a cost when we make something live across a call
that wasn't previously.  I don't think we generally account for that.
> 
> So maybe the safest thing is to go with just the auto-inc-dec patch for
> GCC 10.
I'm certainly willing to go with your judgment on this.  Again I've got
a slight concern about the possibility of unintended consequences. 
THen again we may have positive unintended consequences if we were to
go forward with this patch now.


Jeff