On 09/12/2017 12:47 PM, Richard Sandiford wrote:
> It feels like SECONDARY_MEMORY_NEEDED and SECONDARY_MEMORY_NEEDED_MODE
> should be a single hook that returns the memory mode if memory is needed
> and an empty opt_mode otherwise. The snag is this code in reload.c:
>
> /* If we need a memory location to copy between the two reload regs,
> set it up now. Note that we do the input case before making
> the reload and the output case after. This is due to the
> way reloads are output. */
>
> if (in_p && icode == CODE_FOR_nothing
> && SECONDARY_MEMORY_NEEDED (rclass, reload_class, mode))
> {
> get_secondary_mem (x, reload_mode, opnum, type);
>
> /* We may have just added new reloads. Make sure we add
> the new reload at the end. */
> s_reload = n_reloads;
> }
>
> where we use "mode" to test whether a secondary reload is needed but pass
> "reload_mode" to get_secondary_mem (and thus SECONDARY_MEMORY_NEEDED_MODE).
> This difference appears to come from:
>
> Thu Aug 21 17:56:06 1997 Richard Kenner <ken...@vlsi1.ultra.nyu.edu>
>
> * reload.c (push_secondary_reload): If SECONDARY_MEM_NEEDED,
> call get_secondary_mem for input before adding reload and
> for output after.
> (push_reload): Likewise.
>
> which sounds like it was just moving the code, but also changed the
> get_secondary_mem mode argument from "mode" to "reload_mode".
>
> It seems unlikely that the two modes should be different, but it's not
> obvious which one is right. The corresponding code for output reloads
> uses "mode" consistently, like the input code did before the patch above:
>
> if (! in_p && icode == CODE_FOR_nothing
> && SECONDARY_MEMORY_NEEDED (reload_class, rclass, mode))
> get_secondary_mem (x, mode, opnum, type);
>
> while the main secondary_reload hook uses "reload_mode":
>
> rclass = (enum reg_class) targetm.secondary_reload (in_p, x, reload_class,
> reload_mode, &sri);
>
> The difference only matters for reloads of paradoxical subregs that need
> to go through secondary memory, which is hardly a common case. In the
> end the whole thing seemed too delicate to change, so the patch instead
> just converts the macro to a hook with itscurrent interface.
I wandered my private gcc2 archives, for Aug 1997, but didn't see
anything obviously relevant. I don't have archives for the old gcc-bugs
list -- even if we had them and the change was in response to a bug
reported there I don't think we typically posted anything that would
allow us to map backwards from the change to the bug.
Note that Jim had changed some of this code a bit earlier in a way that
might be relevant:
commit c03001748750bec11636c34e93d77be19da49054
Author: wilson <wilson@138bc75d-0d04-0410-961f-82ee72b054a4>
Date: Wed Sep 25 00:44:04 1996 +0000
(push_secondary_reload): Do strip paradoxical SUBREG
even if reload_class is CLASS_CANNOT_CHANGE_SIZE. Change reload_mode
to mode in SECONDARY_MEMORY_NEEDED and get_secondary_mem calls.
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@12841
138bc75d-0d04-0410-961f-82ee72b054a4
But nothing in the gcc2 archives about that patch either.
I won't even try to guess what's going on here. As you note, the safe
thing to do is preserve current behavior. We can always go back
independently and try to make this consistent and see what falls out.
Given the age, whatever port and attribute of the port in question may
be a dead end in processor architecture and no longer relevant.
>
> There is also a change for LRA. Previously the code was:
>
> if (sclass == NO_REGS && dclass == NO_REGS)
> return false;
> #ifdef SECONDARY_MEMORY_NEEDED
> if (SECONDARY_MEMORY_NEEDED (sclass, dclass, GET_MODE (src))
> #ifdef SECONDARY_MEMORY_NEEDED_MODE
> && ((sclass != NO_REGS && dclass != NO_REGS)
> || GET_MODE (src) != SECONDARY_MEMORY_NEEDED_MODE (GET_MODE
> (src)))
> #endif
> )
> {
> *sec_mem_p = true;
> return false;
> }
> #endif
>
> This allows reloads to and from memory (class == NO_REGS) to use
> secondary memory reloads. It comes from:
>
> 2013-05-24 Vladimir Makarov <vmaka...@redhat.com>
>
> * lra-constraints.c (emit_spill_move): Use smaller mode for
> mem-mem moves.
> (check_and_process_move): Consider mem-reg moves for secondary
> too.
> (curr_insn_transform): Don't lose insns emitted before for
> secondary memory moves.
> (inherit_in_ebb): Mark defined reg. Add usage only if it is not a
> reg set up in the current insn.
>
> But the positioning of the second ifdef meant that defining
> SECONDARY_MEMORY_NEEDED_MODE to its default value was not a no-op:
>
> (1) if a target does define SECONDARY_MEMORY_NEEDED_MODE, only cases
> that need a different memory mode would use secondary memory reloads.
> Cases that need the same memory mode would not use secondary reloads.
>
> (2) if a target doesn't define SECONDARY_MEMORY_NEEDED_MODE, we would
> always use secondary memory reloads for mem<->reg and reg<->mem,
> even though the secondary memory would have the same mode as the
> memory that we're moving to or from.
>
> (1) seems like the correct behaviour, since in (2) there's nothing to
> distinguish the secondary memory from the original; we'd just get
> a redundant mem<->mem move.
>
> The default is different for reload and LRA. For LRA the default is
> to use the original mode, while reload promotes smaller-than-word
> integral modes to word mode:
And what most ports seem to do is avoid that promotion to word_mode!
For example on a 64 bit target, we may need a secondary memory reload
for a 32 bit object which we can often handle just fine. At least
that's the impression I get from scanning the uses.
>
> if (GET_MODE_BITSIZE (mode) < BITS_PER_WORD && INTEGRAL_MODE_P (mode))
> mode = mode_for_size (BITS_PER_WORD,
> GET_MODE_CLASS (mode), 0).require ();
>
> Some of the ports that have switched to LRA seemed to have
> SECONDARY_MEMORY_NEEDED_MDOEs based on the old reload definition,
> and still referred to the reload.c:get_secondary_mem function in
> the comments. The patch just keeps them as-is.
Seems like the safe thing to do, but may be ripe for some cleanups.
>
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
> Also tested by comparing the testsuite assembly output on at least one
> target per CPU directory. OK to install?
>
> Richard
>
>
> 2017-09-12 Richard Sandiford <richard.sandif...@linaro.org>
> Alan Hayward <alan.hayw...@arm.com>
> David Sherwood <david.sherw...@arm.com>
>
> gcc/
> * target.def (secondary_memory_needed_mode): New hook:
> * targhooks.c (default_secondary_memory_needed_mode): Declare.
> * targhooks.h (default_secondary_memory_needed_mode): New function.
> * doc/tm.texi.in (SECONDARY_MEMORY_NEEDED_MODE): Replace with...
> (TARGET_SECONDARY_MEMORY_NEEDED_MODE): ...this.
> * doc/tm.texi: Regenerate.
> * lra-constraints.c (check_and_process_move): Use
> targetm.secondary_memory_needed_mode instead of
> TARGET_SECONDARY_MEMORY_NEEDED_MODE.
> (curr_insn_transform): Likewise.
> * reload.c (get_secondary_mem): Likewise.
> * config/alpha/alpha.h (SECONDARY_MEMORY_NEEDED_MODE): Delete.
> * config/alpha/alpha.c (alpha_secondary_memory_needed_mode): New
> function.
> (TARGET_SECONDARY_MEMORY_NEEDED_MODE): Redefine.
> * config/i386/i386.h (SECONDARY_MEMORY_NEEDED_MODE): Delete.
> * config/i386/i386.c (ix86_secondary_memory_needed_mode): New function.
> (TARGET_SECONDARY_MEMORY_NEEDED_MODE): Redefine.
> * config/powerpcspe/powerpcspe.h (SECONDARY_MEMORY_NEEDED_MODE):
> Delete.
> * config/powerpcspe/powerpcspe-protos.h
> (rs6000_secondary_memory_needed_mode): Delete.
> * config/powerpcspe/powerpcspe.c
> (TARGET_SECONDARY_MEMORY_NEEDED_MODE): Redefine.
> (rs6000_secondary_memory_needed_mode): Make static.
> * config/rs6000/rs6000.h (SECONDARY_MEMORY_NEEDED_MODE): Delete.
> * config/rs6000/rs6000-protos.h (rs6000_secondary_memory_needed_mode):
> Delete.
> * config/rs6000/rs6000.c (TARGET_SECONDARY_MEMORY_NEEDED_MODE):
> Redefine.
> (rs6000_secondary_memory_needed_mode): Make static.
> * config/s390/s390.h (SECONDARY_MEMORY_NEEDED_MODE): Delete.
> * config/s390/s390.c (s390_secondary_memory_needed_mode): New function.
> (TARGET_SECONDARY_MEMORY_NEEDED_MODE): Redefine.
> * config/sparc/sparc.h (SECONDARY_MEMORY_NEEDED_MODE): Delete.
> * config/sparc/sparc.c (TARGET_SECONDARY_MEMORY_NEEDED_MODE):
> Redefine.
> (sparc_secondary_memory_needed_mode): New function.
> * system.h (TARGET_SECONDARY_MEMORY_NEEDED_MODE): Poison.
OK.
jeff
