Re: Is it OK for rtx_addr_can_trap_p_1 to attempt to compute the frame layout? (was Re: [PATCH] Skip re-computing the mips frame info after reload completed)

[Bernd Schmidt <bschmidt@redhat.com>]

On 01/28/2016 12:36 AM, Eric Botcazou wrote:
>> [cc-ing Eric as RTL maintainer]
>
> Sorry for the delay, the message apparently bounced]
>
>> IMO the problem is that rtx_addr_can_trap_p_1 duplicates a large
>> bit of LRA/reload logic:
>>
>> [...]
>>
>> Under the current interface macros like INITIAL_ELIMINATION_OFFSET
>> are expected to trigger the calculation of the target's frame layout
>> (since you need that information to answer the question).
>> To me it seems wrong that we're attempting to call that sort of
>> macro in a query routine like rtx_addr_can_trap_p_1.
>
> I'm a little uncomfortable stepping in here because, while I essentially share
> your objections and was opposed to the patch (I was almost sure that it would
> introduce maintainance issues for no practical benefit), I didn't imagine that
> such a failure mode would have been possible (computing an approximation of
> the frame layout after reload being problematic) so I didn't really object to
> being overruled after seeing Bernd's patch...
>
>> IMO we should cache the information we need@the start of each
>> LRA/reload cycle.  This will be more robust, because there will
>> be no accidental changes to global state either during or after
>> LRA/reload.  It will also be more efficient because
>> rtx_addr_can_trap_p_1 can read the cached variables rather
>> than calling back into the target.
>
> That would be a better design for sure and would eliminate the maintainance
> issues I was originally afraid of.
>
> My recommendation would be to back out Bernd's patch for GCC 6.0 (again, it
> doesn't fix any regression and, more importantly, any bug in real software,
> but only corner case bugs in dumb computer-generated testcases) but with the
> commitment to address the underlying issue for GCC 7.0 and backport the fix to
> GCC 6.x unless really impracticable.  That being said, it's ultimately Jakub
> and Richard's call.

I'm on the fence; I do think the original problem is an issue we should 
fix, but I'm also not terribly happy with the implementation we have 
right now. Besides the issues already mentioned, doesn't it kind of 
assume these offsets are constant (which they definitely aren't, 
consider arg pushes for example)?

I think a better approach might be to just mark accesses at known 
locations in the frame, or arg pushes, as MEM_NOTRAP_P, and consider 
accesses with non-constant or calculated offsets as potentially trapping.


Bernd