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. 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
