On Wed, Sep 12, 2018 at 08:07:41AM -0500, Vlad Lazar wrote: > On 11/09/18 17:53, James Greenhalgh wrote: > > On Mon, Aug 06, 2018 at 11:14:17AM -0500, Vlad Lazar wrote: > >> Hi, > >> > >> The patch adds support for the TARGET_COMPUTE_FRAME_LAYOUT hook on AArch64 > >> and removes unneeded frame layout recalculation. > >> > >> The removed aarch64_layout_frame calls are unnecessary because the > >> functions in which > >> they appear will be called during or after the reload pass in which the > >> TARGET_COMPUTE_FRAME_LAYOUT > >> hook is called. The if statement in aarch64_layout_frame had the purpose > >> of avoiding > >> the extra work from the calls which have been removed and is now redundant. > > > > I'm not sure I understand, I may be missing something as the frame layout > > is complex, but I can't get where I need to be to accept your patch from > > this > > comment. > > > > The check you removed ensures that if we're after reload, and the frame is > > laid out, we do no additional work. That part I understand, and that would > > mean that any post-reload calls were no-ops. Is the argument that all > > users of this code that you eliminate are after reload, and consequently > > would have hit this no-op path? Can you talk me through why each case is > > safe? > > > Thanks for taking a look at the patch. > > Indeed, all the removed calls are happening during or after reload. I'll go > trough all of them > and try to explain the rationale behind. > > aarch64_expand_prologue and aarch64_expand_epilogue are called after the > pro_and_epilogue pass, > which runs after reload where TARGET_COMPUTE_FRAME_LAYOUT is called. > > aarch64_use_return_insn_p checks explicitly for reload_completed at the > beginning of the function > and returns false if reload has not run. So it's safe to remove the call as > the frame layout is > computed by the time it reaches that point. > > aarch64_get_separate_components implements the > TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS hook. > This hook only seems to be used int > shrink_wrap.c:try_shrink_wrapping_separate. The actual origin > of this hook call can be traced back to the pro_and_epilogue pass: > shrink_wrap.c:try_shrink_wrapping_separate <- > function.c:thread_prologue_and_epilogue_insns <- > function.c:rest_of_handle_thread_prologue_and_epilogue (pro_and_epilogue pass > entry point). > Therefore, aarch64_get_separate_components only gets called post reload. > > aarch64_get_separate_components implements the INITIAL_ELIMINATION_OFFSET > hook, which is used in: > 1. rtlanal.c:get_initial_register_offset: Before using the hook it > checks that reload has > been completed. > 2. reload1.c:get_initial_register_offset and > reload1.c:set_initial_elim_offsets: These functions > explicitly call TARGET_COMPUTE_FRAME_LAYOUT before using the hook. > 3. lra-eliminitations.c:update_reg_eliminate: The > TARGET_COMPUTE_FRAME_LAYOUT is, again, called > before the INITIAL_ELIMINATION_OFFSET hook is used. > > I hope this helps make things a bit clearer.
Thanks for this, it is very helpful. The patch is OK for trunk. James > >> gcc/ > >> 2018-08-06 Vlad Lazar <vlad.la...@arm.com> > >> > >> * config/aarch64/aarch64.h (TARGET_COMPUTE_FRAME_LAYOUT): Define. > >> * config/aarch64/aarch64.c (aarch64_expand_prologue): Remove > >> aarch64_layout_frame call. > >> (aarch64_expand_epilogue): Likewise. > >> (aarch64_initial_elimination_offset): Likewise. > >> (aarch64_get_separate_components): Likewise. > >> (aarch64_use_return_insn_p): Likewise. > >> (aarch64_layout_frame): Remove unneeded check. >
