cprop: Don't replace fixed hard regs with pseudos [PR93124]
diff mbox series

Message ID mpt4kwnu2b2.fsf@arm.com
State New
Headers show
Series
  • cprop: Don't replace fixed hard regs with pseudos [PR93124]
Related show

Commit Message

Richard Sandiford Jan. 22, 2020, 12:02 p.m. UTC
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.
---
 gcc/cprop.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Jeff Law Jan. 22, 2020, 7:37 p.m. UTC | #1
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.

Jeff
Richard Sandiford Jan. 23, 2020, 11:43 a.m. UTC | #2
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.

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.

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.

So maybe the safest thing is to go with just the auto-inc-dec patch for
GCC 10.

Thanks,
Richard
Jeff Law Jan. 23, 2020, 8:05 p.m. UTC | #3
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
Richard Sandiford Jan. 24, 2020, 10:17 a.m. UTC | #4
Jeff Law <law@redhat.com> writes:
> 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.

FWIW, I think they're a really useful feature.  E.g. they help when
modelling the lifetimes of multi-hard-reg pseudos that are accessed
via subregs.  Without them, we do a poor job tracking register lifetimes
for things like ST3 and ST4 (g:3ba4ff4130903a3ded931d715a2204bd8834fe60).
We might eventually be able to avoid some of that by using better subreg
tracking, but I suspect there are always going to be cases in which
clobber information inherited from gimple will be needed (or least
work better).

Also, the fact that gimple has essentially the same feature (for
clobbers at least) suggests this might be one of those things that
is bound to be invented if it doesn't already exist. ;-)

Richard

Patch
diff mbox series

diff --git a/gcc/cprop.c b/gcc/cprop.c
index 169ca804e33..5f6446984d8 100644
--- a/gcc/cprop.c
+++ b/gcc/cprop.c
@@ -261,6 +261,18 @@  cprop_reg_p (const_rtx x)
   return REG_P (x) && !HARD_REGISTER_P (x);
 }
 
+/* Return true if it is worth replacing register REGNO with a different
+   register.  */
+
+static bool
+cprop_replace_with_reg_p (unsigned int regno)
+{
+  /* Replacing a fixed register with a pseudo can lead to extra spilling.
+     If REGNO is eliminable, replacing it could also lead to less efficient
+     eliminations.  */
+  return !HARD_REGISTER_NUM_P (regno) || !fixed_regs[regno];
+}
+
 /* Scan SET present in INSN and add an entry to the hash TABLE.
    IMPLICIT is true if it's an implicit set, false otherwise.  */
 
@@ -1098,6 +1110,7 @@  cprop_insn (rtx_insn *insn)
 	  /* Copy propagation.  */
 	  else if (src_reg && cprop_reg_p (src_reg)
 		   && REGNO (src_reg) != regno
+		   && cprop_replace_with_reg_p (regno)
 		   && try_replace_reg (reg_used, src_reg, insn))
 	    {
 	      changed_this_round = changed = 1;
@@ -1198,6 +1211,7 @@  do_local_cprop (rtx x, rtx_insn *insn)
 	  if (cprop_constant_p (this_rtx))
 	    newcnst = this_rtx;
 	  if (cprop_reg_p (this_rtx)
+	      && cprop_replace_with_reg_p (REGNO (x))
 	      /* Don't copy propagate if it has attached REG_EQUIV note.
 		 At this point this only function parameters should have
 		 REG_EQUIV notes and if the argument slot is used somewhere