| Message ID | mpt4kwnu2b2.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | cprop: Don't replace fixed hard regs with pseudos [PR93124] | expand |
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
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
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
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
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