On Thu, 2020-01-23 at 11:43 +0000, Richard Sandiford wrote: > Jeff Law <l...@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.sandif...@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
