Matthew Fortune <matthew.fort...@imgtec.com> writes: > Bernd Edlinger <bernd.edlin...@hotmail.de> writes: >> On 30.12.2015 15:31, Richard Sandiford wrote: >> > I think the problem is deeper than that though. The instructions that >> > are triggering the ICE are only generated by the prologue, so this >> > means that we're trying to lay out the frame again after the prologue >> > has been generated, whereas it really needs to be fixed by then. (And >> > even if recalculating it is a no-op, the operation is still too >> > expensive to be repeated lightly.) Query functions like >> > rtx_addr_can_trap_p(_1) shouldn't really be changing or recalculating >> > the frame layout or other global state. I think we need to find a >> > different way of getting the information. Maybe reload/LRA should use >> > its own structures to calculate the range of "safe" stack and hfp >> > offsets, then store them in crtl for rtx_addr_can_trap_p to use. AIUI, >> > before reload, the only non-trapping uses of sp should be for outgoing >> > arguments, so we can use a test based on the cumulative outgoing >> > arguments size. I don't think the hfp should be used at all before >> > reload, so we could conservatively return -1 for that case. Thanks, >> > Richard >> >> >> Yes, I agree, it _should_ be a no-op, but given the complexity of >> mips_compute_frame_info it is probably better to use cached values after >> reload completed. >> >> Before reload_completed, rtx_addr_can_trap_p is just trying to do an >> initial guess on the stack layout, it may well be possible that some >> important information is still missing here. We will never call the >> target callback from here, before reload_completed. It is however not >> completely impossible that rtx_addr_can_trap_p is called while >> lra/reload is already renaming the registers, see PR66614.
It sounds like we could then have the same kind of problem when reload_in_progress though. I.e. we'd have a predicate function like rtx_addr_can_trap_p changing global state. I realise the symptoms wouldn't be the same as in this PR because the instructions that trigger the ICE are part of the prologue, but it might show up in other ways. E.g. LRA (like reload) loops until it reaches a stable stack layout. If one iteration of the loop does something that changes the stack layout (and so requires another go-round), calling rtx_addr_can_trap_p before the loop restarts would alter the global state unexpectedly. That might trigger an ICE. If we need rtx_addr_can_trap_p to be as accurate as possible (rather than conservative) during LRA/reload, I suppose we should calculate the crtl fields I mentioned during each LRA/reload phase, rather than at the end. >> Of course the required information is already cached in >> cfun->machine->frame, but rtx_addr_can_trap_p can't use the information >> without a target callback. And I just want to avoid to invent a new >> callback for that. Right. I don't think we need or want a new callback. My point was that reload and LRA already compute the offsets you need, so it seems better to cache them there than to try to recalculate them in rtlanal.c. >> I looked at how other targets handle this situation and found that most >> of them either have a more simple frame layout function or they keep the >> previously cached values if they are ever called after reload_completed. >> >> I found, previously mips_compute_frame_info was always called with >> reload_completed=false, either from mips_frame_pointer_required or from >> mips_initial_elimination_offset. Reload seems to call the >> initial_elimination_offset repeatedly and seems to _expect_ the values >> to change from time to time. Yeah. This happens if the first reload attempt fails because there aren't enough free registers or enough spill slots. We then update the frame layout to account for the new spill slots and call-saved register save slots, then try again. >> However it does not seem to expect the >> result from mips_frame_pointer_required to change after reload/LRA has >> started. But as it seems, that may be possible for TARGET_MIPS16 if the >> frame size happens to be very close to 0x7FFF. Well that could be >> another story. What kind of case were you thinking of? >> Anyways when mips_compute_frame_info is called with >> reload_completed=true, we can as well use the cached values, as they >> were immediately before reload_completed. >> >> So what do you think of my new version of the patch? > > Hi Bernd, > > I don't see any problem with this change from a MIPS backend perspective. > As to whether rtx_addr_can_trap_p should be using initial_elimination_offset > then I don't have any particular opinion. > > Richard: Any objections to using this fix so we can solve the PR leaving > debate on the original rtx_addr_can_trap_p as a separate issue? Well, it still feels to me like the patch is papering over the problem. Like Bernd says, we didn't previously call mips_compute_frame_info with reload_completed, and I don't think we should be treating the new behaviour as normal. Thanks, Richard
