Hi Ajit,
On 21/01/2024 19:57, Ajit Agarwal wrote:
>
> Hello All:
>
> New pass to replace adjacent memory addresses lxv with lxvp.
> Added common infrastructure for load store fusion for
> different targets.
Thanks for this, it would be nice to see the load/store pair pass
generalized to multiple targets.
I assume you are targeting GCC 15 for this, as we are in stage 4 at
the moment?
>
> Common routines are refactored in fusion-common.h.
>
> AARCH64 load/store fusion pass is not changed with the
> common infrastructure.
I think any patch to generalize the load/store pair fusion pass should
update the aarch64 code at the same time to use the generic
infrastructure, instead of duplicating the code.
As a general comment, I think we should move as much of the code as
possible to target-independent code, with only the bits that are truly
target-specific (e.g. deciding which modes to allow for a load/store
pair operand) in target code.
In terms of structuring the interface between generic code and target
code, I think it would be pragmatic to use a class with (in some cases,
pure) virtual functions that can be overriden by targets to implement
any target-specific behaviour.
IMO the generic class should be implemented in its own .cc instead of
using a header-only approach. The target code would then define a
derived class which overrides the virtual functions (where necessary)
declared in the generic class, and then instantiate the derived class to
create a target-customized instance of the pass.
A more traditional GCC approach would be to use optabs and target hooks
to customize the behaviour of the pass to handle target-specific
aspects, but:
- Target hooks are quite heavyweight, and we'd potentially have to add
quite a few hooks just for one pass that (at least initially) will
only be used by a couple of targets.
- Using classes allows both sides to easily maintain their own state
and share that state where appropriate.
Nit on naming: I understand you want to move away from ldp_fusion, but
how about pair_fusion or mem_pair_fusion instead of just "fusion" as a
base name? IMO just "fusion" isn't very clear as to what the pass is
trying to achieve.
In general the code could do with a lot more commentary to explain the
rationale for various things / explain the high-level intent of the
code.
Unfortunately I'm not familiar with the DF framework (I've only really
worked with RTL-SSA for the aarch64 pass), so I haven't commented on the
use of that framework, but it would be nice if what you're trying to do
could be done using RTL-SSA instead of using DF directly.
Hopefully Richard S can chime in on those aspects.
My main concerns with the patch at the moment (apart from the code
duplication) is that it looks like:
- The patch removes alias analysis from try_fuse_pair, which is unsafe.
- The patch tries to make its own RTL changes inside
rs6000_gen_load_pair, but it should let fuse_pair make those changes
using RTL-SSA instead.
I've left some more specific (but still mostly high-level) comments below.
>
> For AARCH64 architectures just include "fusion-common.h"
> and target dependent code can be added to that.
>
>
> Alex/Richard:
>
> If you would like me to add for AARCH64 I can do that for AARCH64.
>
> If you would like to do that is fine with me.
>
> Bootstrapped and regtested with powerpc64-linux-gnu.
>
> Improvement in performance is seen with Spec 2017 spec FP benchmarks.
>
> Thanks & Regards
> Ajit
>
> rs6000: New pass for replacement of adjacent lxv with lxvp.
Are you looking to handle stores eventually, out of interest? Looking
at rs6000-vecload-opt.cc:fusion_bb it looks like you're just handling
loads at the moment.
>
> New pass to replace adjacent memory addresses lxv with lxvp.
> Added common infrastructure for load store fusion for
> different targets.
>
> Common routines are refactored in fusion-common.h.
I've just done a very quick scan through this file as it mostly just
looks to be idential to existing code in aarch64-ldp-fusion.cc.
>
> 2024-01-21 Ajit Kumar Agarwal <aagar...@linux.ibm.com>
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000-passes.def: New vecload pass
> before pass_early_remat.
> * config/rs6000/rs6000-vecload-opt.cc: Add new pass.
> * config.gcc: Add new executable.
> * config/rs6000/rs6000-protos.h: Add new prototype for vecload
> pass.
> * config/rs6000/rs6000.cc: Add new prototype for vecload pass.
> * config/rs6000/t-rs6000: Add new rule.
> * fusion-common.h: Add common infrastructure for load store
> fusion that can be shared across different architectures.
> * emit-rtl.cc: Modify assert code.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/powerpc/vecload.C: New test.
> * g++.target/powerpc/vecload1.C: New test.
> * gcc.target/powerpc/mma-builtin-1.c: Modify test.
> ---
> gcc/config.gcc | 4 +-
> gcc/config/rs6000/rs6000-passes.def | 3 +
> gcc/config/rs6000/rs6000-protos.h | 1 +
> gcc/config/rs6000/rs6000-vecload-opt.cc | 1186 ++++++++++++++++
> gcc/config/rs6000/rs6000.cc | 1 +
> gcc/config/rs6000/t-rs6000 | 5 +
> gcc/emit-rtl.cc | 14 +-
> gcc/fusion-common.h | 1195 +++++++++++++++++
> gcc/testsuite/g++.target/powerpc/vecload.C | 15 +
> gcc/testsuite/g++.target/powerpc/vecload1.C | 22 +
> .../gcc.target/powerpc/mma-builtin-1.c | 4 +-
> 11 files changed, 2433 insertions(+), 17 deletions(-)
> create mode 100644 gcc/config/rs6000/rs6000-vecload-opt.cc
> create mode 100644 gcc/fusion-common.h
> create mode 100644 gcc/testsuite/g++.target/powerpc/vecload.C
> create mode 100644 gcc/testsuite/g++.target/powerpc/vecload1.C
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 00355509c92..9bff42cf830 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -518,7 +518,7 @@ or1k*-*-*)
> ;;
> powerpc*-*-*)
> cpu_type=rs6000
> - extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o"
> + extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o
> rs6000-vecload-opt.o"
> extra_objs="${extra_objs} rs6000-call.o rs6000-pcrel-opt.o"
> extra_objs="${extra_objs} rs6000-builtins.o rs6000-builtin.o"
> extra_headers="ppc-asm.h altivec.h htmintrin.h htmxlintrin.h"
> @@ -555,7 +555,7 @@ riscv*)
> ;;
> rs6000*-*-*)
> extra_options="${extra_options} g.opt fused-madd.opt
> rs6000/rs6000-tables.opt"
> - extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o"
> + extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o
> rs6000-vecload-opt.o"
> extra_objs="${extra_objs} rs6000-call.o rs6000-pcrel-opt.o"
> target_gtfiles="$target_gtfiles
> \$(srcdir)/config/rs6000/rs6000-logue.cc
> \$(srcdir)/config/rs6000/rs6000-call.cc"
> target_gtfiles="$target_gtfiles
> \$(srcdir)/config/rs6000/rs6000-pcrel-opt.cc"
> diff --git a/gcc/config/rs6000/rs6000-passes.def
> b/gcc/config/rs6000/rs6000-passes.def
> index 46a0d0b8c56..eb4a65ebe10 100644
> --- a/gcc/config/rs6000/rs6000-passes.def
> +++ b/gcc/config/rs6000/rs6000-passes.def
> @@ -28,6 +28,9 @@ along with GCC; see the file COPYING3. If not see
> The power8 does not have instructions that automaticaly do the byte
> swaps
> for loads and stores. */
> INSERT_PASS_BEFORE (pass_cse, 1, pass_analyze_swaps);
> + /* Pass to replace adjacent memory addresses lxv instruction with lxvp
> + instruction. */
> + INSERT_PASS_BEFORE (pass_early_remat, 1, pass_analyze_vecload);
>
> /* Pass to do the PCREL_OPT optimization that combines the load of an
> external symbol's address along with a single load or store using that
> diff --git a/gcc/config/rs6000/rs6000-protos.h
> b/gcc/config/rs6000/rs6000-protos.h
> index 09a57a806fa..f0a9f36602e 100644
> --- a/gcc/config/rs6000/rs6000-protos.h
> +++ b/gcc/config/rs6000/rs6000-protos.h
> @@ -343,6 +343,7 @@ namespace gcc { class context; }
> class rtl_opt_pass;
>
> extern rtl_opt_pass *make_pass_analyze_swaps (gcc::context *);
> +extern rtl_opt_pass *make_pass_analyze_vecload (gcc::context *);
> extern rtl_opt_pass *make_pass_pcrel_opt (gcc::context *);
> extern bool rs6000_sum_of_two_registers_p (const_rtx expr);
> extern bool rs6000_quadword_masked_address_p (const_rtx exp);
> diff --git a/gcc/config/rs6000/rs6000-vecload-opt.cc
> b/gcc/config/rs6000/rs6000-vecload-opt.cc
> new file mode 100644
> index 00000000000..cebadad97d8
> --- /dev/null
> +++ b/gcc/config/rs6000/rs6000-vecload-opt.cc
> @@ -0,0 +1,1186 @@
> +/* Subroutines used to replace lxv with lxvp
> + for TARGET_POWER10 and TARGET_VSX,
> +
> + Copyright (C) 2020-2023 Free Software Foundation, Inc.
> + Contributed by Ajit Kumar Agarwal <aagar...@linux.ibm.com>.
> +
> + This file is part of GCC.
> +
> + GCC is free software; you can redistribute it and/or modify it
> + under the terms of the GNU General Public License as published
> + by the Free Software Foundation; either version 3, or (at your
> + option) any later version.
> +
> + GCC is distributed in the hope that it will be useful, but WITHOUT
> + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> + License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with GCC; see the file COPYING3. If not see
> + <http://www.gnu.org/licenses/>. */
> +
> +#define IN_TARGET_CODE 1
> +#define INCLUDE_ALGORITHM
> +#define INCLUDE_FUNCTIONAL
> +#define INCLUDE_LIST
> +#define INCLUDE_TYPE_TRAITS
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "backend.h"
> +#include "target.h"
> +#include "rtl.h"
> +#include "memmodel.h"
> +#include "emit-rtl.h"
> +#include "tree-pass.h"
> +#include "df.h"
> +#include "dumpfile.h"
> +#include "rs6000-internal.h"
> +#include "rs6000-protos.h"
> +#include "fusion-common.h"
> +
> +/* Return false if dependent rtx LOC is SUBREG. */
> +static bool
> +is_feasible (rtx_insn *insn)
Can you explain the rationale for this function?
I'm not really sure what it's trying to achieve.
> +{
> + df_ref use;
> + df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + if (!def_link || !def_link->ref || DF_REF_IS_ARTIFICIAL
> (def_link->ref))
> + continue;
> + while (def_link && def_link->ref)
> + {
> + rtx *loc = DF_REF_LOC (def_link->ref);
> + if (!loc || *loc == NULL_RTX)
> + return false;
> + if (GET_CODE (*loc) == SUBREG)
> + return false;
> + def_link = def_link->next;
> + }
> + }
> + return true;
> +}
> +
> +/* df_scan_rescan the unspec instruction where operands
> + are reversed. */
> +void set_rescan_for_unspec (rtx_insn *insn)
> +{
> + df_ref use;
> + df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
> + rtx_insn *select_insn2;
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + while (def_link && def_link->ref)
> + {
> + select_insn2 = DF_REF_INSN (def_link->ref);
> + rtx set = single_set (select_insn2);
> +
> + if (set == NULL_RTX)
> + return;
> +
> + if (set != NULL_RTX)
> + {
> + rtx op0 = SET_SRC (set);
> + if (GET_CODE (op0) != UNSPEC)
> + return;
> +
> + if (GET_CODE (op0) == VEC_SELECT
> + && GET_CODE (XEXP (op0, 1)) == PARALLEL)
> + return;
> +
> + if (GET_CODE (op0) == UNSPEC)
> + df_insn_rescan (select_insn2);
> + }
> + def_link = def_link->next;
> + }
> + }
> +}
> +
> +/* Return dependent UNSPEC instruction. */
> +rtx_insn *get_rtx_UNSPEC (rtx_insn *insn)
> +{
> + df_ref use;
> + df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
> + rtx_insn *select_insn2;
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + while (def_link && def_link->ref)
> + {
> + select_insn2 = DF_REF_INSN (def_link->ref);
> + rtx set = single_set (select_insn2);
> +
> + if (set == NULL_RTX)
> + return 0;
> +
> + if (set != NULL_RTX)
This condition is always true due to the early return above.
> + {
> + rtx op0 = SET_SRC (set);
> +
> + if (GET_CODE (op0) == UNSPEC)
> + return select_insn2;
> + }
> + def_link = def_link->next;
> + }
> + }
> + return 0;
> +}
> +
> +/* Replace identified lxv with lxvp.
> + Bail out if following condition are true:
> +
> + - dependent instruction of load is vec_select instruction,
Please can you explain the rationale for this condition?
Do I understand correctly that you want to bail out if a use of a
register loaded by either insn occurs in a vec_select rtx? If so, why?
AIUI, since we shouldn't change register dataflow in the pass, I wonder
if you could check this condition once in track_access, instead of
checking it for every pair formation attempt?
> +
> + - machine mode of unspec is not same as machine mode
> + of lxv instruction.
> +
> + - dependent instruction is not unspec.
Likewise, please explain the rationale for this.
> +
> + - Source operand of unspec is eq instruction. */
Same here.
> +
> +rtx
> +rs6000_gen_load_pair (rtx_insn *insn1, rtx_insn *insn2)
> +{
I think this function is named in a rather misleading way, and I think
it is trying to do too much at once.
Based on the name of the function, I would expect it to just produce the
appropriate RTL pattern for a load pair given the two candidate loads.
But it actually tries to do a bunch of analysis to determine if the pair
can be formed, and then inserts a new insn and deletes the original
candidate insns.
Any changing of the RTL should be done by RTL-SSA in fuse_pair.
> + rtx body = PATTERN (insn1);
> + rtx src_exp = SET_SRC (body);
> + rtx dest_exp = SET_DEST (body);
> + rtx lxv;
> + rtx insn2_body = PATTERN (insn2);
> +
> + rtx insn2_src_exp = SET_SRC (insn2_body);
> +
> + if (GET_MODE (src_exp) != GET_MODE (SET_SRC (insn2_body)))
> + return NULL_RTX;
IIUC this should hold from generic RTL rules, so shouldn't be needed.
> +
> + if (GET_MODE (dest_exp) == TImode)
> + return NULL_RTX;
> +
> + if (!ALTIVEC_OR_VSX_VECTOR_MODE (GET_MODE (dest_exp)))
> + return NULL_RTX;
Validation of the modes should have happened much earlier (in
track_access), so shouldn't be necessary at this point.
> +
> + if (!is_feasible (insn1))
> + return NULL_RTX;
> +
> + if (!is_feasible (insn2))
> + return NULL_RTX;
I can't really comment on this as I don't understand the intent behind
is_feasible, but AFAICT it seems to do some dataflow analysis (looking
at uses/defs). That should ideally have been done at the latest in
try_fuse_pair (before we get to fuse_pair), and shouldn't be done when
we're trying to generate the final RTL pattern for the pair.
Given that these seem to be properties of the indiviudal insns it might
even be possible to check this much earlier (when first considering the
individual accesses in track_access).
> +
> + for (rtx note = REG_NOTES (insn1); note; note = XEXP (note, 1))
> + if (REG_NOTE_KIND (note) == REG_EQUAL
> + || REG_NOTE_KIND (note) == REG_EQUIV)
> + return NULL_RTX;
Can you explain why you're punting if the candidate insns have these
notes? Did you see a performance improvement with that change? It
might be something we want to consider doing for aarch64 too if running
before RA (in which case we could do it in the generic code).
> +
> + int no_dep = 0;
> + df_ref use;
> + df_insn_info *insn_info = DF_INSN_INFO_GET (insn1);
> + rtx_insn *select_insn2;
> +
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + while (def_link && def_link->ref)
> + {
> + select_insn2 = DF_REF_INSN (def_link->ref);
> + rtx set = single_set (select_insn2);
> +
> + if (set == NULL_RTX)
> + return NULL_RTX;
> +
> + if (set != NULL_RTX)
As above, I think this condition is redundant due to the early return
immediately above.
> + {
> + rtx op0 = SET_SRC (set);
> +
> + if (GET_CODE (op0) != UNSPEC)
> + return NULL_RTX;
> +
> + if (GET_CODE (op0) == VEC_SELECT
> + && GET_CODE (XEXP (op0, 1)) == PARALLEL)
> + return NULL_RTX;
> +
> + if (GET_CODE (op0) == UNSPEC)
> + {
> + if (GET_MODE (op0) != XOmode
> + && GET_MODE (op0) != GET_MODE (dest_exp))
> + return NULL_RTX;
> +
> + int nvecs = XVECLEN (op0, 0);
> + for (int i = 0; i < nvecs; i++)
> + {
> + rtx op;
> + op = XVECEXP (op0, 0, i);
> +
> + if (GET_MODE (op) == OOmode)
> + return NULL_RTX;
> + if (GET_CODE (op) == EQ)
> + return NULL_RTX;
> + }
> + }
> + ++no_dep;
> + }
> + def_link = def_link->next;
> + }
> + }
> +
> + rtx_insn *insn = get_rtx_UNSPEC (insn1);
> +
> + if (insn && insn == get_rtx_UNSPEC (insn2) && no_dep == 1)
> + return NULL_RTX;
> +
> + int regoff;
> + rtx src;
> + rtx addr = XEXP (src_exp, 0);
> +
> + if (GET_CODE (addr) == PLUS
> + && XEXP (addr, 1) && CONST_INT_P (XEXP (addr, 1)))
> + {
> + regoff = 0;
> + src = simplify_gen_subreg (GET_MODE (dest_exp),
> + dest_exp, GET_MODE (dest_exp),
> + regoff);
> + }
> + else
> + {
> + regoff = INTVAL (CONST0_RTX (SImode)) *
> + GET_MODE_SIZE (GET_MODE (dest_exp));
> + src = simplify_gen_subreg (GET_MODE (dest_exp),
> + dest_exp, GET_MODE (dest_exp),
> + regoff);
> + }
> + insn_info = DF_INSN_INFO_GET (insn2);
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + if (!def_link || !def_link->ref || DF_REF_IS_ARTIFICIAL
> (def_link->ref))
> + continue;
> + while (def_link && def_link->ref)
> + {
> + rtx *loc = DF_REF_LOC (def_link->ref);
> + *loc = src;
> + def_link = def_link->next;
> + }
> + }
> +
> + int regoff1;
> + rtx src1;
> + addr = XEXP (insn2_src_exp, 0);
> +
> + if (GET_CODE (addr) == PLUS
> + && XEXP (addr, 1)
> + && CONST_INT_P (XEXP(addr, 1)))
> + {
> + regoff1 = 16;
> + src1 = simplify_gen_subreg (GET_MODE (dest_exp),
> + dest_exp, GET_MODE (dest_exp),
> + regoff1);
> + }
> + else
> + {
> + regoff1 = INTVAL (CONST0_RTX (SImode)) * GET_MODE_SIZE (GET_MODE
> (dest_exp));//V16QImode);
> + src1 = simplify_gen_subreg (GET_MODE (dest_exp),
> + dest_exp, GET_MODE (dest_exp),
> + regoff1);
> + }
> +
> + insn_info = DF_INSN_INFO_GET (insn1);
> + FOR_EACH_INSN_INFO_DEF (use, insn_info)
> + {
> + struct df_link *def_link = DF_REF_CHAIN (use);
> + if (!def_link || !def_link->ref || DF_REF_IS_ARTIFICIAL
> (def_link->ref))
> + continue;
> + while (def_link && def_link->ref)
> + {
> + rtx *loc = DF_REF_LOC (def_link->ref);
> + PUT_MODE_RAW (*loc, OOmode);
> + *loc = src1;
> + def_link = def_link->next;
> + }
> + }
> +
> + set_rescan_for_unspec (insn1);
> + set_rescan_for_unspec (insn2);
> + df_insn_rescan (insn1);
> + df_insn_rescan (insn2);
> +
> + PUT_MODE_RAW (src_exp, OOmode);
> + PUT_MODE_RAW (dest_exp, OOmode);
I don't think you should be updating the original RTL directly like
this, but instead return a new pattern which will ultimately get passed
to validate_unshare_change in fuse_pair (i.e. let fuse_pair update the
RTL).
> + lxv = gen_movoo (dest_exp, src_exp);
> + rtx_insn *new_insn = emit_insn_before (lxv, insn1);
Likewise, instead of emitting a new insn, this should let fuse_pair
perform the change.
> + set_block_for_insn (new_insn, BLOCK_FOR_INSN (insn1));
> + df_insn_rescan (new_insn);
> +
> + if (dump_file)
> + {
> + unsigned int new_uid = INSN_UID (new_insn);
> + fprintf (dump_file, "Replacing lxv %d with lxvp %d\n",
> + INSN_UID (insn1), new_uid);
> + print_rtl_single (dump_file, new_insn);
> + print_rtl_single (dump_file, insn1);
> + print_rtl_single (dump_file, insn2);
> +
> + }
> +
> + df_insn_delete (insn1);
> + remove_insn (insn1);
> + df_insn_delete (insn2);
> + remove_insn (insn2);
> + insn1->set_deleted ();
> + insn2->set_deleted ();
Likewise, I don't think you should be deleting the original insns here,
but you should let RTL-SSA do this in the generic code instead (i.e. in
fuse_pair).
> + return lxv;
> +}
> +
> +// LEFT_LIST and RIGHT_LIST are lists of candidate instructions where all
> insns
> +// in LEFT_LIST are known to be adjacent to those in RIGHT_LIST.
> +//
> +// This function traverses the resulting 2D matrix of possible pair
> candidates
> +// and attempts to merge them into pairs.
> +//
> +// The algorithm is straightforward: if we consider a combined list of
> +// candidates X obtained by merging LEFT_LIST and RIGHT_LIST in program
> order,
> +// then we advance through X until we reach a crossing point (where X[i] and
> +// X[i+1] come from different source lists).
> +//
> +// At this point we know X[i] and X[i+1] are adjacent accesses, and we try to
> +// fuse them into a pair. If this succeeds, we remove X[i] and X[i+1] from
> +// their original lists and continue as above.
> +//
> +// In the failure case, we advance through the source list containing X[i]
> and
> +// continue as above (proceeding to the next crossing point).
> +//
> +// The rationale for skipping over groups of consecutive candidates from the
> +// same source list is as follows:
> +//
> +// In the store case, the insns in the group can't be re-ordered over each
> +// other as they are guaranteed to store to the same location, so we're
> +// guaranteed not to lose opportunities by doing this.
> +//
> +// In the load case, subsequent loads from the same location are either
> +// redundant (in which case they should have been cleaned up by an earlier
> +// optimization pass) or there is an intervening aliasing hazard, in which
> case
> +// we can't re-order them anyway, so provided earlier passes have cleaned up
> +// redundant loads, we shouldn't miss opportunities by doing this.
> +void
> +fusion_bb_info::merge_pairs (insn_list_t &left_list,
> + insn_list_t &right_list,
> + bool load_p,
> + unsigned access_size)
> +{
This function should be in the generic code, not in target code.
Was there a reason you moved it to target code?
When moving code from aarch64-ldp-fusion.cc, I think it would be better
if you left the code style unchanged and only made changes where there
is a need for a functional change. For example ...
> + if (dump_file)
> + {
> + fprintf (dump_file, "merge_pairs [L=%d], cand vecs ", load_p);
> + dump_insn_list (dump_file, left_list);
> + fprintf (dump_file, " x ");
> + dump_insn_list (dump_file, right_list);
> + fprintf (dump_file, "\n");
> + }
> +
> + auto iter_l = left_list.begin ();
> + auto iter_r = right_list.begin ();
> + while (iter_l != left_list.end () && iter_r != right_list.end ())
> + {
> + auto left = *iter_l;
> + auto right = *iter_r;
... here you introduce new left and right locals, but AFAICT there isn't
a need for this (no functional change).
> +
> + auto next_l = std::next (iter_l);
> + auto next_r = std::next (iter_r);
> +
> + if ((*left) < (*right)
> + && next_l != (left_list.end ())
> + && **next_l < *right)
> + iter_l = next_l;
> + else if ((*right) < (*left)
> + && next_r != (right_list.end ())
> + && **next_r < *left)
> + iter_r = next_r;
> + else if (try_fuse_pair (load_p, access_size, *iter_l, *iter_r))
> //left, right))
> + {
> + left_list.erase (iter_l);
> + iter_l = next_l;
> + right_list.erase (iter_r);
> + iter_r = next_r;
> + }
> + else if (*left < *right)
> + iter_l = next_l;
> + else
> + iter_r = next_r;
> + }
> +
> +}
> +
> +
> +
> +// Main function to begin pair discovery. Given a memory access INSN,
> +// determine whether it could be a candidate for fusing into an lxvp/stvxp,
> +// and if so, track it in the appropriate data structure for this basic
> +// block. LOAD_P is true if the access is a load, and MEM is the mem
> +// rtx that occurs in INSN.
> +void
> +fusion_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> +{
This function should also be in generic code, not in target code.
I can see that you want to customize a target-specific aspect ...
> + // We can't combine volatile MEMs, so punt on these.
> + if (MEM_VOLATILE_P (mem))
> + return;
> +
> + // Ignore writeback accesses if the param says to do so.
> + if (GET_RTX_CLASS (GET_CODE (XEXP (mem, 0))) == RTX_AUTOINC)
> + return;
> +
> + const machine_mode mem_mode = GET_MODE (mem);
> +
> + rtx reg_op = XEXP (PATTERN (insn->rtl ()), !load_p);
> +
> + if (!ALTIVEC_OR_VSX_VECTOR_MODE (GET_MODE (reg_op)))
> + return;
... here. But I think this should be done either using a target hook
or ideally using a virtual function which is overriden by target code.
For rs6000, would it be enough to validate the mode of the mem instead
of looking at the mode of reg_op? Then we could have a single function,
e.g.:
bool pair_fusion::pair_operand_mode_ok_p (machine_mode mode);
which for rs6000 would return:
ALTIVEC_OR_VSX_VECTOR_MODE (mode);
and for aarch64 would return:
ldp_operand_mode_ok_p (mode);
and in track_access (from generic code), you would do:
if (!pair_operand_mode_ok_p (mem_mode))
return;
note that, at least for aarch64, the reason that we validate the mem mode
instead of the register mode is that the register operand may be a modeless
const_int in the store case.
> +
> + const bool fpsimd_op_p = (ALTIVEC_OR_VSX_VECTOR_MODE (GET_MODE (reg_op)));
IIUC, this boolean is always true due to the above check. So it is
better to just set it to a constant (doesn't matter which) if you only
want to handle vector modes for rs6000. As above, this should be done
through a virtual function, where for aarch64 we return the condition in
aarch64-ldp-fusion.cc:
const bool fpsimd_op_p
= reload_completed
? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
: (GET_MODE_CLASS (mem_mode) != MODE_INT
&& (load_p || !aarch64_const_zero_rtx_p (reg_op)));
and for rs6000 you can just return false (assuming you don't want to segregate
accesses using this bit).
> +
> + const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode);
> + const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
> +
> + if (track_via_mem_expr (insn, mem, lfs))
> + return;
> +
> + poly_int64 mem_off;
> + rtx addr = XEXP (mem, 0);
> + const bool autoinc_p = GET_RTX_CLASS (GET_CODE (addr)) == RTX_AUTOINC;
> + rtx base = load_strip_offset (mem, &mem_off);
> + if (!REG_P (base))
> + return;
> +
> + // Need to calculate two (possibly different) offsets:
> + // - Offset at which the access occurs.
> + // - Offset of the new base def.
> + poly_int64 access_off;
> + if (autoinc_p && any_post_modify_p (addr))
> + access_off = 0;
> + else
> + access_off = mem_off;
> +
> + poly_int64 new_def_off = mem_off;
> +
> + // Punt on accesses relative to eliminable regs. Since we don't know the
> + // elimination offset pre-RA, we should postpone forming pairs on such
> + // accesses until after RA.
> + //
> + // As it stands, addresses with offsets in range for LDR but not
> + // in range for LDP/STP are currently reloaded inefficiently,
> + // ending up with a separate base register for each pair.
> + //
> + // In theory LRA should make use of
> + // targetm.legitimize_address_displacement to promote sharing of
> + // bases among multiple (nearby) address reloads, but the current
> + // LRA code returns early from process_address_1 for operands that
> + // satisfy "m", even if they don't satisfy the real (relaxed) address
> + // constraint; this early return means we never get to the code
> + // that calls targetm.legitimize_address_displacement.
> + //
> + // So for now, it's better to punt when we can't be sure that the
> + // offset is in range for LDP/STP. Out-of-range cases can then be
> + // handled after RA by the out-of-range LDP/STP peepholes. Eventually, it
> + // would be nice to handle known out-of-range opportunities in the
> + // pass itself (for stack accesses, this would be in the post-RA pass).
> + if (!reload_completed
> + && (REGNO (base) == FRAME_POINTER_REGNUM
> + || REGNO (base) == ARG_POINTER_REGNUM))
> + return;
> +
> + // Now need to find def of base register.
> + use_info *base_use = find_access (insn->uses (), REGNO (base));
> + gcc_assert (base_use);
> + def_info *base_def = base_use->def ();
> + if (!base_def)
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "base register (regno %d) of insn %d is undefined",
> + REGNO (base), insn->uid ());
> + return;
> + }
> +
> + alt_base *canon_base = canon_base_map.get (base_def);
> + if (canon_base)
> + {
> + // Express this as the combined offset from the canonical base.
> + base_def = canon_base->base;
> + new_def_off += canon_base->offset;
> + access_off += canon_base->offset;
> + }
> +
> + if (autoinc_p)
> + {
> + auto def = find_access (insn->defs (), REGNO (base));
> + gcc_assert (def);
> +
> + // Record that DEF = BASE_DEF + MEM_OFF.
> + if (dump_file)
> + {
> + pretty_printer pp;
> + pp_access (&pp, def, 0);
> + pp_string (&pp, " = ");
> + pp_access (&pp, base_def, 0);
> + fprintf (dump_file, "[bb %u] recording %s + ",
> + m_bb->index (), pp_formatted_text (&pp));
> + print_dec (new_def_off, dump_file);
> + fprintf (dump_file, "\n");
> + }
> +
> + alt_base base_rec { base_def, new_def_off };
> + if (canon_base_map.put (def, base_rec))
> + gcc_unreachable (); // Base defs should be unique.
> + }
> +
> + // Punt on misaligned offsets. LDP/STP require offsets to be a multiple of
> + // the access size.
> + if (!multiple_p (mem_off, mem_size))
> + return;
> +
> + const auto key = std::make_pair (base_def, encode_lfs (lfs));
> + access_group &group = def_map.get_or_insert (key, NULL);
> + auto alloc = [&](access_record *access) { return node_alloc (access); };
> + group.track (alloc, access_off, insn);
> + if (dump_file)
> + {
> + pretty_printer pp;
> + pp_access (&pp, base_def, 0);
> +
> + fprintf (dump_file, "[bb %u] tracking insn %d via %s",
> + m_bb->index (), insn->uid (), pp_formatted_text (&pp));
> + fprintf (dump_file,
> + " [L=%d, WB=%d, FP=%d, %smode, off=",
> + lfs.load_p, autoinc_p, lfs.fpsimd_p, mode_name[mem_mode]);
> + print_dec (access_off, dump_file);
> + fprintf (dump_file, "]\n");
> + }
> +}
> +
> +
> +// Given two adjacent memory accesses of the same size, I1 and I2, try
> +// and see if we can merge them into a lxvp or stvxp.
> +//
> +// ACCESS_SIZE gives the (common) size of a single access, LOAD_P is true
> +// if the accesses are both loads, otherwise they are both stores.
> +bool
> +fusion_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
> + insn_info *i1, insn_info *i2)
Likewise, I think this function should be in generic code, not target code.
> +{
> + if (dump_file)
> + fprintf (dump_file, "analyzing pair (load=%d): (%d,%d)\n",
> + load_p, i1->uid (), i2->uid ());
> +
> + insn_info *insns[2];
> + bool reversed = false;
> + if (*i1 < *i2)
> + {
> + insns[0] = i1;
> + insns[1] = i2;
> + }
> + else
> + {
> + insns[0] = i2;
> + insns[1] = i1;
> + reversed = true;
> + }
> +
> + rtx cand_mems[2];
> + rtx reg_ops[2];
> + rtx pats[2];
> + for (int i = 0; i < 2; i++)
> + {
> + pats[i] = PATTERN (insns[i]->rtl ());
> + cand_mems[i] = XEXP (pats[i], load_p);
> + reg_ops[i] = XEXP (pats[i], !load_p);
> + }
> +
> + if (load_p && reg_overlap_mentioned_p (reg_ops[0], reg_ops[1]))
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "punting on lxvp due to reg conflcits (%d,%d)\n",
I understand that you might not want to use "ldp" in the generic code,
but how about "punting on load pair" instead?
> + insns[0]->uid (), insns[1]->uid ());
> + return false;
> + }
> +
> + if (cfun->can_throw_non_call_exceptions
> + && find_reg_note (insns[0]->rtl (), REG_EH_REGION, NULL_RTX)
> + && find_reg_note (insns[1]->rtl (), REG_EH_REGION, NULL_RTX))
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "can't combine insns with EH side effects (%d,%d)\n",
> + insns[0]->uid (), insns[1]->uid ());
> + return false;
> + }
> +
> + auto_vec<base_cand, 2> base_cands (2);
> +
> + int writeback = get_viable_bases (insns, base_cands, cand_mems,
> + access_size, reversed);
> + if (base_cands.is_empty ())
> + {
> + if (dump_file)
> + fprintf (dump_file, "no viable base for pair (%d,%d)\n",
> + insns[0]->uid (), insns[1]->uid ());
> + return false;
> + }
> +
> + // Punt on frame-related insns with writeback. We probably won't see
> + // these in practice, but this is conservative and ensures we don't
> + // have to worry about these later on.
> + if (writeback && (RTX_FRAME_RELATED_P (i1->rtl ())
> + || RTX_FRAME_RELATED_P (i2->rtl ())))
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "rejecting pair (%d,%d): frame-related insn with writeback\n",
> + i1->uid (), i2->uid ());
> + return false;
> + }
> +
> + rtx *ignore = &XEXP (pats[1], load_p);
> + for (auto use : insns[1]->uses ())
> + if (!use->is_mem ()
> + && refers_to_regno_p (use->regno (), use->regno () + 1, pats[1], ignore)
> + && use->def () && use->def ()->insn () == insns[0])
> + {
> + // N.B. we allow a true dependence on the base address, as this
> + // happens in the case of auto-inc accesses. Consider a post-increment
> + // load followed by a regular indexed load, for example.
> + if (dump_file)
> + fprintf (dump_file,
> + "%d has non-address true dependence on %d, rejecting pair\n",
> + insns[1]->uid (), insns[0]->uid ());
> + return false;
> + }
> +
> + unsigned i = 0;
> + while (i < base_cands.length ())
> + {
> + base_cand &cand = base_cands[i];
> +
> + rtx *ignore[2] = {};
> + for (int j = 0; j < 2; j++)
> + if (cand.from_insn == !j)
> + ignore[j] = &XEXP (cand_mems[j], 0);
> +
> + insn_info *h = first_hazard_after (insns[0], ignore[0]);
> + if (h && *h <= *insns[1])
> + cand.hazards[0] = h;
> +
> + h = latest_hazard_before (insns[1], ignore[1]);
> + if (h && *h >= *insns[0])
> + cand.hazards[1] = h;
> +
> + if (!cand.viable ())
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "pair (%d,%d): rejecting base %d due to dataflow "
> + "hazards (%d,%d)\n",
> + insns[0]->uid (),
> + insns[1]->uid (),
> + cand.def->regno (),
> + cand.hazards[0]->uid (),
> + cand.hazards[1]->uid ());
> +
> + base_cands.ordered_remove (i);
> + }
> + else
> + i++;
> + }
> +
> + if (base_cands.is_empty ())
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "can't form pair (%d,%d) due to dataflow hazards\n",
> + insns[0]->uid (), insns[1]->uid ());
> + return false;
> + }
> +
> + //insn_info *alias_hazards[4] = {};
It looks like you've just commented out / removed the alias analysis (I don't
see a call to do_alias_analysis). How is that supposed to work?
> +
> + // First def of memory after the first insn, and last def of memory
> + // before the second insn, respectively.
> + def_info *mem_defs[2] = {};
> + if (load_p)
> + {
> + if (!MEM_READONLY_P (cand_mems[0]))
> + {
> + mem_defs[0] = memory_access (insns[0]->uses ())->def ();
> + gcc_checking_assert (mem_defs[0]);
> + mem_defs[0] = mem_defs[0]->next_def ();
> + }
> + if (!MEM_READONLY_P (cand_mems[1]))
> + {
> + mem_defs[1] = memory_access (insns[1]->uses ())->def ();
> + gcc_checking_assert (mem_defs[1]);
> + }
> + }
> + else
> + {
> + mem_defs[0] = memory_access (insns[0]->defs ())->next_def ();
> + mem_defs[1] = memory_access (insns[1]->defs ())->prev_def ();
> + gcc_checking_assert (mem_defs[0]);
> + gcc_checking_assert (mem_defs[1]);
> + }
> +
> + //auto tombstone_p = [&](insn_info *insn) -> bool {
> + // return m_emitted_tombstone
> +// && bitmap_bit_p (&m_tombstone_bitmap, insn->uid ());
> + // };
> +
> + if (base_cands.is_empty ())
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "cannot form pair (%d,%d) due to alias/dataflow hazards",
> + insns[0]->uid (), insns[1]->uid ());
> +
> + return false;
> + }
> +
> + base_cand *base = &base_cands[0];
> + insn_range_info range (insns[0], insns[1]);
> + // If the second insn can throw, narrow the move range to exactly that
> insn.
> + // This prevents us trying to move the second insn from the end of the BB.
> + if (cfun->can_throw_non_call_exceptions
> + && find_reg_note (insns[1]->rtl (), REG_EH_REGION, NULL_RTX))
> + {
> + gcc_assert (range.includes (insns[1]));
> + range = insn_range_info (insns[1]);
> + }
> +
> + // Placement strategy: push loads down and pull stores up, this should
> + // help register pressure by reducing live ranges.
> + if (load_p)
> + range.first = range.last;
> + else
> + range.last = range.first;
> + return fuse_pair (load_p, access_size, writeback,
> + i1, i2, *base, range);
> +}
> +// Try and actually fuse the pair given by insns I1 and I2.
> +//
> +// Here we've done enough analysis to know this is safe, we only
> +// reject the pair at this stage if either the tuning policy says to,
> +// or recog fails on the final pair insn.
> +//
> +// LOAD_P is true for loads, ACCESS_SIZE gives the access size of each
> +// candidate insn. Bit i of WRITEBACK is set if the ith insn (in program
> +// order) uses writeback.
> +//
> +// BASE gives the chosen base candidate for the pair and MOVE_RANGE is
> +// a singleton range which says where to place the pair.
> +bool
> +fusion_bb_info::fuse_pair (bool load_p,
> + unsigned access_size,
> + int writeback,
> + insn_info *i1, insn_info *i2,
> + base_cand &base,
> + const insn_range_info &move_range)
> +{
Again, I think this function should be in generic code, not in target
code.
> + auto attempt = crtl->ssa->new_change_attempt ();
> +
> + auto make_change = [&attempt](insn_info *insn)
> + {
> + return crtl->ssa->change_alloc<insn_change> (attempt, insn);
> + };
> + auto make_delete = [&attempt](insn_info *insn)
> + {
> + return crtl->ssa->change_alloc<insn_change> (attempt,
> + insn,
> + insn_change::DELETE);
> + };
> +
> + if (*i1 > *i2)
> + return false;
> +
> + insn_info *first = (*i1 < *i2) ? i1 : i2;
> + insn_info *second = (first == i1) ? i2 : i1;
> +
> + insn_info *insns[2] = { first, second };
> +
> + auto_vec<insn_change *, 4> changes (4);
> + auto_vec<int, 2> tombstone_uids (2);
> +
> + rtx pats[2] = {
> + PATTERN (first->rtl ()),
> + PATTERN (second->rtl ())
> + };
> +
> + use_array input_uses[2] = { first->uses (), second->uses () };
> + def_array input_defs[2] = { first->defs (), second->defs () };
> +
> + int changed_insn = -1;
> + if (base.from_insn != -1)
> + {
> + // If we're not already using a shared base, we need
> + // to re-write one of the accesses to use the base from
> + // the other insn.
> + gcc_checking_assert (base.from_insn == 0 || base.from_insn == 1);
> + changed_insn = !base.from_insn;
> +
> + rtx base_pat = pats[base.from_insn];
> + rtx change_pat = pats[changed_insn];
> + rtx base_mem = XEXP (base_pat, load_p);
> + rtx change_mem = XEXP (change_pat, load_p);
> +
> + const bool lower_base_p = (insns[base.from_insn] == i1);
> + HOST_WIDE_INT adjust_amt = access_size;
> + if (!lower_base_p)
> + adjust_amt *= -1;
> +
> + rtx change_reg = XEXP (change_pat, !load_p);
> + machine_mode mode_for_mem = GET_MODE (change_mem);
> + rtx effective_base = drop_writeback (base_mem);
> + rtx new_mem = adjust_address_nv (effective_base,
> + mode_for_mem,
> + adjust_amt);
> + rtx new_set = load_p
> + ? gen_rtx_SET (change_reg, new_mem)
> + : gen_rtx_SET (new_mem, change_reg);
> +
> + pats[changed_insn] = new_set;
> +
> + auto keep_use = [&](use_info *u)
> + {
> + return refers_to_regno_p (u->regno (), u->regno () + 1,
> + change_pat, &XEXP (change_pat, load_p));
> + };
> +
> + // Drop any uses that only occur in the old address.
> + input_uses[changed_insn] = filter_accesses (attempt,
> + input_uses[changed_insn],
> + keep_use);
> + }
> +
> + rtx writeback_effect = NULL_RTX;
> + if (writeback)
> + writeback_effect = extract_writebacks (load_p, pats, changed_insn);
> +
> + const auto base_regno = base.def->regno ();
> +
> + if (base.from_insn == -1 && (writeback & 1))
> + {
> + // If the first of the candidate insns had a writeback form, we'll
> need to
> + // drop the use of the updated base register from the second insn's
> uses.
> + //
> + // N.B. we needn't worry about the base register occurring as a store
> + // operand, as we checked that there was no non-address true dependence
> + // between the insns in try_fuse_pair.
> + gcc_checking_assert (find_access (input_uses[1], base_regno));
> + input_uses[1] = check_remove_regno_access (attempt,
> + input_uses[1],
> + base_regno);
> + }
> +
> + // Go through and drop uses that only occur in register notes,
> + // as we won't be preserving those.
> + for (int i = 0; i < 2; i++)
> + {
> + auto rti = insns[i]->rtl ();
> + if (!REG_NOTES (rti))
> + continue;
> +
> + input_uses[i] = remove_note_accesses (attempt, input_uses[i]);
> + }
> +
> + // So far the patterns have been in instruction order,
> + // now we want them in offset order.
> + if (i1 != first)
> + std::swap (pats[0], pats[1]);
> +
> + poly_int64 offsets[2];
> + for (int i = 0; i < 2; i++)
> + {
> + rtx mem = XEXP (pats[i], load_p);
> + gcc_checking_assert (MEM_P (mem));
> + rtx base = strip_offset (XEXP (mem, 0), offsets + i);
> + gcc_checking_assert (REG_P (base));
> + gcc_checking_assert (base_regno == REGNO (base));
> + }
> +
> + // If either of the original insns had writeback, but the resulting pair
> insn
> + // does not (can happen e.g. in the lxvp edge case above, or if the
> writeback
> + // effects cancel out), then drop the def(s) of the base register as
> + // appropriate.
> + //
> + // Also drop the first def in the case that both of the original insns had
> + // writeback. The second def could well have uses, but the first def
> should
> + // only be used by the second insn (and we dropped that use above).
> + for (int i = 0; i < 2; i++)
> + if ((!writeback_effect && (writeback & (1 << i)))
> + || (i == 0 && writeback == 3))
> + input_defs[i] = check_remove_regno_access (attempt,
> + input_defs[i],
> + base_regno);
> +
> + // If we don't currently have a writeback pair, and we don't have
> + // a load that clobbers the base register, look for a trailing destructive
> + // update of the base register and try and fold it in to make this into a
> + // writeback pair.
> + insn_info *trailing_add = nullptr;
I can see you've dropped the call to find_trailing_add here.
If you don't want to handle writeback pairs, that's fine, but I think
that should be controlled with a target-controlled virtual function.
Effectively you can just replace uses of the param aarch64_ldp_writeback with a
call to such a function, then the target can control which writeback
opportunities we handle, if any. For aarch64 this would just return
the result of the param for the time being.
> +
> + rtx reg_notes = combine_reg_notes (first, second, load_p);
> +
> + rtx pair_pat = NULL_RTX;
> +
> + if (load_p) {
> + pair_pat = rs6000_gen_load_pair ((first->rtl ()), (second->rtl ()));
> + if (pair_pat == NULL_RTX)
> + return false;
> + }
> + insn_change *pair_change = nullptr;
> + auto set_pair_pat = [pair_pat,reg_notes](insn_change *change) {
> + rtx_insn *rti = change->insn ()->rtl ();
> + validate_unshare_change (rti, &PATTERN (rti), pair_pat, true);
> + validate_change (rti, ®_NOTES (rti), reg_notes, true);
> + };
> +
> + if (load_p)
> + {
> + changes.quick_push (make_delete (first));
> + pair_change = make_change (second);
> + changes.quick_push (pair_change);
> +
> + pair_change->move_range = move_range;
> + pair_change->new_defs = merge_access_arrays (attempt,
> + input_defs[0],
> + input_defs[1]);
> + gcc_assert (pair_change->new_defs.is_valid ());
> +
> + pair_change->new_uses
> + = merge_access_arrays (attempt,
> + drop_memory_access (input_uses[0]),
> + drop_memory_access (input_uses[1]));
> + gcc_assert (pair_change->new_uses.is_valid ());
> + set_pair_pat (pair_change);
> + }
> + else
> + {
> + insn_info *store_to_change = decide_store_strategy (first, second,
> + move_range);
> +
> + if (store_to_change && dump_file)
> + fprintf (dump_file, " stvx: re-purposing store %d\n",
> + store_to_change->uid ());
> +
> + insn_change *change;
> + for (int i = 0; i < 2; i++)
> + {
> + change = make_change (insns[i]);
> + if (insns[i] == store_to_change)
> + {
> + set_pair_pat (change);
> + change->new_uses = merge_access_arrays (attempt,
> + input_uses[0],
> + input_uses[1]);
> + auto d1 = drop_memory_access (input_defs[0]);
> + auto d2 = drop_memory_access (input_defs[1]);
> + change->new_defs = merge_access_arrays (attempt, d1, d2);
> + gcc_assert (change->new_defs.is_valid ());
> + def_info *stxvp_def = memory_access (store_to_change->defs ());
> + change->new_defs = insert_access (attempt,
> + stxvp_def,
> + change->new_defs);
> + gcc_assert (change->new_defs.is_valid ());
> + change->move_range = move_range;
> + pair_change = change;
> + }
> + else
> + {
> + // Note that we are turning this insn into a tombstone,
> + // we need to keep track of these if we go ahead with the
> + // change.
> + tombstone_uids.quick_push (insns[i]->uid ());
> + rtx_insn *rti = insns[i]->rtl ();
> + validate_change (rti, &PATTERN (rti), gen_tombstone (), true);
> + validate_change (rti, ®_NOTES (rti), NULL_RTX, true);
> + change->new_uses = use_array (nullptr, 0);
> + }
> + gcc_assert (change->new_uses.is_valid ());
> + changes.quick_push (change);
> + }
> +
> + if (!store_to_change)
> + {
> + // Tricky case. Cannot re-purpose existing insns for stp.
> + // Need to insert new insn.
> + if (dump_file)
> + fprintf (dump_file,
> + " stp fusion: cannot re-purpose candidate stores\n");
> +
> + auto new_insn = crtl->ssa->create_insn (attempt, INSN, pair_pat);
> + change = make_change (new_insn);
> + change->move_range = move_range;
> + change->new_uses = merge_access_arrays (attempt,
> + input_uses[0],
> + input_uses[1]);
> + gcc_assert (change->new_uses.is_valid ());
> +
> + auto d1 = drop_memory_access (input_defs[0]);
> + auto d2 = drop_memory_access (input_defs[1]);
> + change->new_defs = merge_access_arrays (attempt, d1, d2);
> + gcc_assert (change->new_defs.is_valid ());
> +
> + auto new_set = crtl->ssa->create_set (attempt, new_insn, memory);
> + change->new_defs = insert_access (attempt, new_set,
> + change->new_defs);
> + gcc_assert (change->new_defs.is_valid ());
> + changes.safe_insert (1, change);
> + pair_change = change;
> + }
> + }
> +
> + if (trailing_add)
> + changes.quick_push (make_delete (trailing_add));
> +
> + auto n_changes = changes.length ();
> + gcc_checking_assert (n_changes >= 2 && n_changes <= 4);
> +
> + gcc_assert (crtl->ssa->verify_insn_changes (changes));
> +
> + cancel_changes (0);
Can you explain why you changed this to unconditionally cancel the
changes instead of letting RTL-SSA perform them?
> +
> + confirm_change_group ();
> +
> + gcc_checking_assert (tombstone_uids.length () <= 2);
> + for (auto uid : tombstone_uids)
> + track_tombstone (uid);
> +
> + return true;
> +}
> +
> +void fusion_bb (bb_info *bb)
> +{
Likewise, I think this function should be in generic code, not in target
code. Note for aarch64 we have:
const bool track_loads
= aarch64_tune_params.ldp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
const bool track_stores
= aarch64_tune_params.stp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
I think these can just be hidden behind target-controlled virtual
functions, so you'd have something like:
bool pair_fusion::should_fuse_stores ();
bool pair_fusion::should_fuse_loads ();
(or a single function that returns multiple bitfields if you like).
then for aarch64 you would return:
aarch64_tune_params.ldp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
for the load case, and for rs6000 you would return true (just based on
what's below). Presumably since you don't handle stores below, you'd
want to return false in the store case for rs6000 code (at least
initially).
As I mentioned above, the aarch64 version of this function also tries
to promote existing pairs to writeback pairs, again that would be
controlled by the same function call that replaces uses of
the param aarch64_ldp_writeback.
> + fusion_bb_info bb_state (bb);
> +
> + for (auto insn : bb->nondebug_insns ())
> + {
> + rtx_insn *rti = insn->rtl ();
> +
> + if (!rti || !INSN_P (rti))
> + continue;
> +
> + rtx pat = PATTERN (rti);
> +
> + if (GET_CODE (pat) != SET)
> + continue;
> +
> + if (MEM_P (XEXP (pat, 1)))
> + bb_state.track_access (insn, true, XEXP (pat, 1));
> + }
> +
> + bb_state.transform ();
> + bb_state.cleanup_tombstones ();
> +}
> +
> +static void
> +fusion_init ()
> +{
> + df_set_flags (DF_RD_PRUNE_DEAD_DEFS);
> + df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);
> + df_analyze ();
> + df_set_flags (DF_DEFER_INSN_RESCAN);
> + /* Rebuild ud- and du-chains. */
> + df_remove_problem (df_chain);
> + df_process_deferred_rescans ();
> + df_set_flags (DF_RD_PRUNE_DEAD_DEFS);
> + df_chain_add_problem (DF_DU_CHAIN | DF_UD_CHAIN);
> + df_analyze ();
> + df_set_flags (DF_DEFER_INSN_RESCAN);
Is there a reason you can't use RTL-SSA for the def-use information?
IMO it would be cleaner to use RTL-SSA as the existing aarch64 pass does
(and the refactored generic code presumably will do).
> + calculate_dominance_info (CDI_DOMINATORS);
> + crtl->ssa = new rtl_ssa::function_info (cfun);
> +}
> +
> +static void
> +fusion_destroy ()
> +{
> + if (crtl->ssa->perform_pending_updates ())
> + cleanup_cfg (0);
> +
> + free_dominance_info (CDI_DOMINATORS);
> + delete crtl->ssa;
> + crtl->ssa = nullptr;
> +}
> +
> +// Iterate over the accesses in GROUP, looking for adjacent sets
> +// of accesses. If we find two sets of adjacent accesses, call
> +// merge_pairs.
> +void
> +fusion_bb_info::transform_for_base (int encoded_lfs,
> + access_group &group)
Again, I think this function should be in generic code.
> +{
> + const auto lfs = decode_lfs (encoded_lfs);
> + const unsigned access_size = lfs.size;
> +
> + bool skip_next = true;
> + access_record *prev_access = nullptr;
> +
> + for (auto &access : group.list)
> + {
> + if (skip_next)
> + skip_next = false;
> + else if (known_eq (access.offset, prev_access->offset + access_size))
> + {
> + merge_pairs (prev_access->cand_insns,
> + access.cand_insns,
> + lfs.load_p,
> + access_size);
> + skip_next = true;
> + }
> + prev_access = &access;
> + }
> +}
> +
> +void rs6000_analyze_vecload ()
> +{
> + fusion_init ();
> +
> + for (auto bb : crtl->ssa->bbs ())
> + fusion_bb (bb);
> +
> + fusion_destroy ();
> +}
> +
> +const pass_data pass_data_analyze_vecload =
> +{
> + RTL_PASS, /* type */
> + "vecload", /* name */
> + OPTGROUP_NONE, /* optinfo_flags */
> + TV_NONE, /* tv_id */
> + 0, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> + TODO_df_finish, /* todo_flags_finish */
> +};
> +
> +class pass_analyze_vecload : public rtl_opt_pass
> +{
> +public:
> + pass_analyze_vecload(gcc::context *ctxt)
> + : rtl_opt_pass(pass_data_analyze_vecload, ctxt)
> + {}
> +
> + /* opt_pass methods: */
> + bool gate (function *)
> + {
> + return (optimize > 0 && TARGET_VSX && TARGET_POWER10);
> + }
> +
> + unsigned int execute (function *) final override
> + {
> + rs6000_analyze_vecload ();
> + return 0;
> + }
> +}; // class pass_analyze_vecload
> +
> +rtl_opt_pass *
> +make_pass_analyze_vecload (gcc::context *ctxt)
> +{
> + return new pass_analyze_vecload (ctxt);
> +}
> +
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 5d975dab921..e36946c44ea 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -1178,6 +1178,7 @@ static bool rs6000_secondary_reload_move (enum
> rs6000_reg_type,
> secondary_reload_info *,
> bool);
> rtl_opt_pass *make_pass_analyze_swaps (gcc::context*);
> +rtl_opt_pass *make_pass_analyze_vecload (gcc::context*);
>
> /* Hash table stuff for keeping track of TOC entries. */
>
> diff --git a/gcc/config/rs6000/t-rs6000 b/gcc/config/rs6000/t-rs6000
> index b3ce09d523b..7664830f8ab 100644
> --- a/gcc/config/rs6000/t-rs6000
> +++ b/gcc/config/rs6000/t-rs6000
> @@ -35,6 +35,11 @@ rs6000-p8swap.o: $(srcdir)/config/rs6000/rs6000-p8swap.cc
> $(COMPILE) $<
> $(POSTCOMPILE)
>
> +rs6000-vecload-opt.o: $(srcdir)/config/rs6000/rs6000-vecload-opt.cc
> + $(COMPILE) $<
> + $(POSTCOMPILE)
> +
> +
> rs6000-d.o: $(srcdir)/config/rs6000/rs6000-d.cc
> $(COMPILE) $<
> $(POSTCOMPILE)
> diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
> index 1856fa4884f..ffc47a6eaa0 100644
> --- a/gcc/emit-rtl.cc
> +++ b/gcc/emit-rtl.cc
> @@ -921,7 +921,7 @@ validate_subreg (machine_mode omode, machine_mode imode,
> return false;
>
> /* The subreg offset cannot be outside the inner object. */
> - if (maybe_ge (offset, isize))
> + if (maybe_gt (offset, isize))
> return false;
Can you explain why this change is needed?
>
> poly_uint64 regsize = REGMODE_NATURAL_SIZE (imode);
> @@ -3035,18 +3035,6 @@ verify_rtx_sharing (rtx orig, rtx insn)
> break;
> }
>
> - /* This rtx may not be shared. If it has already been seen,
> - replace it with a copy of itself. */
> - if (flag_checking && RTX_FLAG (x, used))
> - {
> - error ("invalid rtl sharing found in the insn");
> - debug_rtx (insn);
> - error ("shared rtx");
> - debug_rtx (x);
> - internal_error ("internal consistency failure");
> - }
> - gcc_assert (!RTX_FLAG (x, used));
> -
I think the need to make this change is likely indicative of a problem
in the pass, and the pass should be fixed instead of changing
verify_rtx_sharing.
> RTX_FLAG (x, used) = 1;
>
> /* Now scan the subexpressions recursively. */
> diff --git a/gcc/fusion-common.h b/gcc/fusion-common.h
> new file mode 100644
> index 00000000000..82f1d6ef032
> --- /dev/null
> +++ b/gcc/fusion-common.h
> @@ -0,0 +1,1195 @@
> +// Common infrastructure for Load/Store Pair fusion optimization pass.
> +// Copyright (C) 2023-2024 Free Software Foundation, Inc.
> +//
> +// This file is part of GCC.
> +//
> +// GCC is free software; you can redistribute it and/or modify it
> +// under the terms of the GNU General Public License as published by
> +// the Free Software Foundation; either version 3, or (at your option)
> +// any later version.
> +//
> +// GCC is distributed in the hope that it will be useful, but
> +// WITHOUT ANY WARRANTY; without even the implied warranty of
> +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +// General Public License for more details.
> +//
> +// You should have received a copy of the GNU General Public License
> +// along with GCC; see the file COPYING3. If not see
> +// <http://www.gnu.org/licenses/>.
> +
> +#define INCLUDE_ALGORITHM
> +#define INCLUDE_FUNCTIONAL
> +#define INCLUDE_LIST
> +#define INCLUDE_TYPE_TRAITS
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "backend.h"
> +#include "rtl.h"
> +#include "df.h"
> +#include "rtl-iter.h"
> +#include "rtl-ssa.h"
> +#include "cfgcleanup.h"
> +#include "tree-pass.h"
> +#include "ordered-hash-map.h"
> +#include "tree-dfa.h"
> +#include "fold-const.h"
> +#include "tree-hash-traits.h"
> +#include "print-tree.h"
> +#include "insn-attr.h"
> +
> +using namespace rtl_ssa;
> +
> +static constexpr HOST_WIDE_INT LDP_IMM_BITS = 7;
> +static constexpr HOST_WIDE_INT LDP_IMM_SIGN_BIT = (1 << (LDP_IMM_BITS - 1));
> +static constexpr HOST_WIDE_INT LDP_MAX_IMM = LDP_IMM_SIGN_BIT - 1;
> +static constexpr HOST_WIDE_INT LDP_MIN_IMM = -LDP_MAX_IMM - 1;
Note we would not want these aarch64-specific constants in generic code,
they would need to be target-configurable (in fact we might just want a
virtual function / target hook to validate offsets instead).
> +
> +// We pack these fields (load_p, fpsimd_p, and size) into an integer
> +// (LFS) which we use as part of the key into the main hash tables.
> +//
> +// The idea is that we group candidates together only if they agree on
> +// the fields below. Candidates that disagree on any of these
> +// properties shouldn't be merged together.
> +struct lfs_fields
> +{
> + bool load_p;
> + bool fpsimd_p;
> + unsigned size;
> +};
> +
> +using insn_list_t = std::list<insn_info *>;
> +using insn_iter_t = insn_list_t::iterator;
> +
> +// Information about the accesses at a given offset from a particular
> +// base. Stored in an access_group, see below.
> +struct access_record
> +{
> + poly_int64 offset;
> + std::list<insn_info *> cand_insns;
> + std::list<access_record>::iterator place;
> +
> + access_record (poly_int64 off) : offset (off) {}
> +};
> +
> +// A group of accesses where adjacent accesses could be ldp/stp
> +// or lxvp/stxvp candidates. The splay tree supports efficient
Perhaps better to spell out "load pair or store pair" instead of listing
the various instruction mnenomics for different targets.
> +// insertion, while the list supports efficient iteration.
> +struct access_group
> +{
> + splay_tree<access_record *> tree;
> + std::list<access_record> list;
> +
> + template<typename Alloc>
> + inline void track (Alloc node_alloc, poly_int64 offset, insn_info *insn);
> +};
> +
> +// Information about a potential base candidate, used in try_fuse_pair.
> +// There may be zero, one, or two viable RTL bases for a given pair.
> +struct base_cand
> +{
> + // DEF is the def of the base register to be used by the pair.
> + def_info *def;
> +
> + // FROM_INSN is -1 if the base candidate is already shared by both
> + // candidate insns. Otherwise it holds the index of the insn from
> + // which the base originated.
> + //
> + // In the case that the base is shared, either DEF is already used
> + // by both candidate accesses, or both accesses see different versions
> + // of the same regno, in which case DEF is the def consumed by the
> + // first candidate access.
> + int from_insn;
> +
> + // To form a pair, we do so by moving the first access down and the second
> + // access up. To determine where to form the pair, and whether or not
> + // it is safe to form the pair, we track instructions which cannot be
> + // re-ordered past due to either dataflow or alias hazards.
> + //
> + // Since we allow changing the base used by an access, the choice of
> + // base can change which instructions act as re-ordering hazards for
> + // this pair (due to different dataflow). We store the initial
> + // dataflow hazards for this choice of base candidate in HAZARDS.
> + //
> + // These hazards act as re-ordering barriers to each candidate insn
> + // respectively, in program order.
> + //
> + // Later on, when we take alias analysis into account, we narrow
> + // HAZARDS accordingly.
> + insn_info *hazards[2];
> +
> + base_cand (def_info *def, int insn)
> + : def (def), from_insn (insn), hazards {nullptr, nullptr} {}
> +
> + base_cand (def_info *def) : base_cand (def, -1) {}
> +
> + // Test if this base candidate is viable according to HAZARDS.
> + bool viable () const
> + {
> + return !hazards[0] || !hazards[1] || (*hazards[0] > *hazards[1]);
> + }
> +};
> +
> +// Information about an alternate base. For a def_info D, it may
> +// instead be expressed as D = BASE + OFFSET.
> +struct alt_base
> +{
> + def_info *base;
> + poly_int64 offset;
> +};
> +
> +// State used by the pass for a given basic block.
> +struct fusion_bb_info
> +{
> + using def_hash = nofree_ptr_hash<def_info>;
> + using expr_key_t = pair_hash<tree_operand_hash, int_hash<int, -1, -2>>;
> + using def_key_t = pair_hash<def_hash, int_hash<int, -1, -2>>;
> +
> + // Map of <tree base, LFS> -> access_group.
> + ordered_hash_map<expr_key_t, access_group> expr_map;
> +
> + // Map of <RTL-SSA def_info *, LFS> -> access_group.
> + ordered_hash_map<def_key_t, access_group> def_map;
> +
> + // Given the def_info for an RTL base register, express it as an offset
> from
> + // some canonical base instead.
> + //
> + // Canonicalizing bases in this way allows us to identify adjacent accesses
> + // even if they see different base register defs.
> + hash_map<def_hash, alt_base> canon_base_map;
> +
> + static const size_t obstack_alignment = sizeof (void *);
> + bb_info *m_bb;
> +
> + fusion_bb_info (bb_info *bb) : m_bb (bb), m_emitted_tombstone (false)
> + {
> + obstack_specify_allocation (&m_obstack, OBSTACK_CHUNK_SIZE,
> + obstack_alignment, obstack_chunk_alloc,
> + obstack_chunk_free);
> + }
> + ~fusion_bb_info ()
> + {
> + obstack_free (&m_obstack, nullptr);
> +
> + if (m_emitted_tombstone)
> + {
> + bitmap_release (&m_tombstone_bitmap);
> + bitmap_obstack_release (&m_bitmap_obstack);
> + }
> + }
> +
> + inline void track_access (insn_info *, bool load, rtx mem);
> + inline void transform ();
> + inline void cleanup_tombstones ();
> +
> +private:
> + obstack m_obstack;
> +
> + // State for keeping track of tombstone insns emitted for this BB.
> + bitmap_obstack m_bitmap_obstack;
> + bitmap_head m_tombstone_bitmap;
> + bool m_emitted_tombstone;
> +
> + inline splay_tree_node<access_record *> *node_alloc (access_record *);
> +
> + template<typename Map>
> + inline void traverse_base_map (Map &map);
> + inline void transform_for_base (int load_size, access_group &group);
> +
> + void merge_pairs (insn_list_t &, insn_list_t &,
> + bool load_p, unsigned access_size);
> +
> + bool try_fuse_pair (bool load_p, unsigned access_size,
> + insn_info *i1, insn_info *i2);
> +
> + bool fuse_pair (bool load_p, unsigned access_size,
> + int writeback,
> + insn_info *i1, insn_info *i2,
> + base_cand &base,
> + const insn_range_info &move_range);
> +
> + inline void track_tombstone (int uid);
> +
> + inline bool track_via_mem_expr (insn_info *, rtx mem, lfs_fields lfs);
> +};
> +
> +splay_tree_node<access_record *> *fusion_bb_info::node_alloc (access_record
> *access)
> +{
> + using T = splay_tree_node<access_record *>;
> + void *addr = obstack_alloc (&m_obstack, sizeof (T));
> + return new (addr) T (access);
> +}
> +
> +// Given a mem MEM, if the address has side effects, return a MEM that
> accesses
> +// the same address but without the side effects. Otherwise, return
> +// MEM unchanged.
> +static rtx
> +drop_writeback (rtx mem)
> +{
> + rtx addr = XEXP (mem, 0);
> +
> + if (!side_effects_p (addr))
> + return mem;
> +
> + switch (GET_CODE (addr))
> + {
> + case PRE_MODIFY:
> + addr = XEXP (addr, 1);
> + break;
> + case POST_MODIFY:
> + case POST_INC:
> + case POST_DEC:
> + addr = XEXP (addr, 0);
> + break;
> + case PRE_INC:
> + case PRE_DEC:
> + {
> + poly_int64 adjustment = GET_MODE_SIZE (GET_MODE (mem));
> + if (GET_CODE (addr) == PRE_DEC)
> + adjustment *= -1;
> + addr = plus_constant (GET_MODE (addr), XEXP (addr, 0), adjustment);
> + break;
> + }
> + default:
> + gcc_unreachable ();
> + }
> +
> + return change_address (mem, GET_MODE (mem), addr);
> +}
> +
> +// Convenience wrapper around strip_offset that can also look through
> +// RTX_AUTOINC addresses. The interface is like strip_offset except we take
> a
> +// MEM so that we know the mode of the access.
> +static rtx
> +load_strip_offset (rtx mem, poly_int64 *offset)
Nit: naming. How about pair_fusion_strip_offset instead?
> +{
> + rtx addr = XEXP (mem, 0);
> +
> + switch (GET_CODE (addr))
> + {
> + case PRE_MODIFY:
> + case POST_MODIFY:
> + addr = strip_offset (XEXP (addr, 1), offset);
> + gcc_checking_assert (REG_P (addr));
> + gcc_checking_assert (rtx_equal_p (XEXP (XEXP (mem, 0), 0), addr));
> + break;
> + case PRE_INC:
> + case POST_INC:
> + addr = XEXP (addr, 0);
> + *offset = GET_MODE_SIZE (GET_MODE (mem));
> + gcc_checking_assert (REG_P (addr));
> + break;
> + case PRE_DEC:
> + case POST_DEC:
> + addr = XEXP (addr, 0);
> + *offset = -GET_MODE_SIZE (GET_MODE (mem));
> + gcc_checking_assert (REG_P (addr));
> + break;
> +
> + default:
> + addr = strip_offset (addr, offset);
> + }
> +
> + return addr;
> +}
> +
> +// Return true if X is a PRE_{INC,DEC,MODIFY} rtx.
> +static bool
> +any_pre_modify_p (rtx x)
> +{
> + const auto code = GET_CODE (x);
> + return code == PRE_INC || code == PRE_DEC || code == PRE_MODIFY;
> +}
> +
> +// Return true if X is a POST_{INC,DEC,MODIFY} rtx.
> +static bool
> +any_post_modify_p (rtx x)
> +{
> + const auto code = GET_CODE (x);
> + return code == POST_INC || code == POST_DEC || code == POST_MODIFY;
> +}
> +// Given LFS (load_p, fpsimd_p, size) fields in FIELDS, encode these
> +// into an integer for use as a hash table key.
> +static int
> +encode_lfs (lfs_fields fields)
> +{
> + int size_log2 = exact_log2 (fields.size);
> + //gcc_checking_assert (size_log2 >= 2 && size_log2 <= 4);
> + return ((int)fields.load_p << 3)
> + | ((int)fields.fpsimd_p << 2)
> + | (size_log2 - 2);
> +}
> +
> +// Inverse of encode_lfs.
> +static lfs_fields
> +decode_lfs (int lfs)
> +{
> + bool load_p = (lfs & (1 << 3));
> + bool fpsimd_p = (lfs & (1 << 2));
> + unsigned size = 1U << ((lfs & 3) + 2);
> + return { load_p, fpsimd_p, size };
> +}
> +// Track the access INSN at offset OFFSET in this access group.
> +// ALLOC_NODE is used to allocate splay tree nodes.
> +template<typename Alloc>
> +void
> +access_group::track (Alloc alloc_node, poly_int64 offset, insn_info *insn)
> +{
> + auto insert_before = [&](std::list<access_record>::iterator after)
> + {
> + auto it = list.emplace (after, offset);
> + it->cand_insns.push_back (insn);
> + it->place = it;
> + return &*it;
> + };
> +
> + if (!list.size ())
> + {
> + auto access = insert_before (list.end ());
> + tree.insert_max_node (alloc_node (access));
> + return;
> + }
> +
> + auto compare = [&](splay_tree_node<access_record *> *node)
> + {
> + return compare_sizes_for_sort (offset, node->value ()->offset);
> + };
> + auto result = tree.lookup (compare);
> + splay_tree_node<access_record *> *node = tree.root ();
> + if (result == 0)
> + node->value ()->cand_insns.push_back (insn);
> + else
> + {
> + auto it = node->value ()->place;
> + auto after = (result > 0) ? std::next (it) : it;
> + auto access = insert_before (after);
> + tree.insert_child (node, result > 0, alloc_node (access));
> + }
> +}
> +// Given a candidate access INSN (with mem MEM), see if it has a suitable
> +// MEM_EXPR base (i.e. a tree decl) relative to which we can track the
> access.
> +// LFS is used as part of the key to the hash table, see track_access.
> +bool
> +fusion_bb_info::track_via_mem_expr (insn_info *insn, rtx mem, lfs_fields lfs)
> +{
> + if (!MEM_EXPR (mem) || !MEM_OFFSET_KNOWN_P (mem))
> + return false;
> +
> + poly_int64 offset;
> + tree base_expr = get_addr_base_and_unit_offset (MEM_EXPR (mem),
> + &offset);
> + if (!base_expr || !DECL_P (base_expr))
> + return false;
> +
> + offset += MEM_OFFSET (mem);
> +
> + const machine_mode mem_mode = GET_MODE (mem);
> + const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode);//.to_constant ();
Why the .to_constant () comment here?
I hope these comments are useful. Please feel free to reach out if you have any
questions / get stuck with anything. I'm often around on IRC.
Thanks,
Alex
> +
> + // Punt on misaligned offsets. LDP/STP instructions require offsets to be
> a
> + // multiple of the access size, and we believe that misaligned offsets on
> + // MEM_EXPR bases are likely to lead to misaligned offsets w.r.t. RTL
> bases.
> + if (!multiple_p (offset, mem_size))
> + return false;
> +
> + const auto key = std::make_pair (base_expr, encode_lfs (lfs));
> + access_group &group = expr_map.get_or_insert (key, NULL);
> + auto alloc = [&](access_record *access) { return node_alloc (access); };
> + group.track (alloc, offset, insn);
> +
> + if (dump_file)
> + {
> + fprintf (dump_file, "[bb %u] tracking insn %d via ",
> + m_bb->index (), insn->uid ());
> + print_node_brief (dump_file, "mem expr", base_expr, 0);
> + fprintf (dump_file, " [L=%d FP=%d, %smode, off=",
> + lfs.load_p, lfs.fpsimd_p, mode_name[mem_mode]);
> + print_dec (offset, dump_file);
> + fprintf (dump_file, "]\n");
> + }
> +
> + return true;
> +}
> +
> +// Dummy predicate that never ignores any insns.
> +static bool no_ignore (insn_info *) { return false; }
> +
> +// Return the latest dataflow hazard before INSN.
> +//
> +// If IGNORE is non-NULL, this points to a sub-rtx which we should ignore for
> +// dataflow purposes. This is needed when considering changing the RTL base
> of
> +// an access discovered through a MEM_EXPR base.
> +//
> +// If IGNORE_INSN is non-NULL, we should further ignore any hazards arising
> +// from that insn.
> +//
> +// N.B. we ignore any defs/uses of memory here as we deal with that
> separately,
> +// making use of alias disambiguation.
> +static insn_info *
> +latest_hazard_before (insn_info *insn, rtx *ignore,
> + insn_info *ignore_insn = nullptr)
> +{
> + insn_info *result = nullptr;
> +
> + // If the insn can throw then it is at the end of a BB and we can't
> + // move it, model this by recording a hazard in the previous insn
> + // which will prevent moving the insn up.
> + if (cfun->can_throw_non_call_exceptions
> + && find_reg_note (insn->rtl (), REG_EH_REGION, NULL_RTX))
> + return insn->prev_nondebug_insn ();
> +
> + // Return true if we registered the hazard.
> + auto hazard = [&](insn_info *h) -> bool
> + {
> + gcc_checking_assert (*h < *insn);
> + if (h == ignore_insn)
> + return false;
> +
> + if (!result || *h > *result)
> + result = h;
> +
> + return true;
> + };
> +
> + rtx pat = PATTERN (insn->rtl ());
> + auto ignore_use = [&](use_info *u)
> + {
> + if (u->is_mem ())
> + return true;
> +
> + return !refers_to_regno_p (u->regno (), u->regno () + 1, pat, ignore);
> + };
> +
> + // Find defs of uses in INSN (RaW).
> + for (auto use : insn->uses ())
> + if (!ignore_use (use) && use->def ())
> + hazard (use->def ()->insn ());
> +
> + // Find previous defs (WaW) or previous uses (WaR) of defs in INSN.
> + for (auto def : insn->defs ())
> + {
> + if (def->is_mem ())
> + continue;
> +
> + if (def->prev_def ())
> + {
> + hazard (def->prev_def ()->insn ()); // WaW
> +
> + auto set = dyn_cast<set_info *> (def->prev_def ());
> + if (set && set->has_nondebug_insn_uses ())
> + for (auto use : set->reverse_nondebug_insn_uses ())
> + if (use->insn () != insn && hazard (use->insn ())) // WaR
> + break;
> + }
> +
> + if (!HARD_REGISTER_NUM_P (def->regno ()))
> + continue;
> +
> + // Also need to check backwards for call clobbers (WaW).
> + for (auto call_group : def->ebb ()->call_clobbers ())
> + {
> + if (!call_group->clobbers (def->resource ()))
> + continue;
> +
> + auto clobber_insn = prev_call_clobbers_ignoring (*call_group,
> + def->insn (),
> + no_ignore);
> + if (clobber_insn)
> + hazard (clobber_insn);
> + }
> +
> + }
> +
> + return result;
> +}
> +
> +// Return the first dataflow hazard after INSN.
> +//
> +// If IGNORE is non-NULL, this points to a sub-rtx which we should ignore for
> +// dataflow purposes. This is needed when considering changing the RTL base
> of
> +// an access discovered through a MEM_EXPR base.
> +//
> +// N.B. we ignore any defs/uses of memory here as we deal with that
> separately,
> +// making use of alias disambiguation.
> +static insn_info *
> +first_hazard_after (insn_info *insn, rtx *ignore)
> +{
> + insn_info *result = nullptr;
> + auto hazard = [insn, &result](insn_info *h)
> + {
> + gcc_checking_assert (*h > *insn);
> + if (!result || *h < *result)
> + result = h;
> + };
> +
> + rtx pat = PATTERN (insn->rtl ());
> + auto ignore_use = [&](use_info *u)
> + {
> + if (u->is_mem ())
> + return true;
> +
> + return !refers_to_regno_p (u->regno (), u->regno () + 1, pat, ignore);
> + };
> +
> + for (auto def : insn->defs ())
> + {
> + if (def->is_mem ())
> + continue;
> +
> + if (def->next_def ())
> + hazard (def->next_def ()->insn ()); // WaW
> +
> + auto set = dyn_cast<set_info *> (def);
> + if (set && set->has_nondebug_insn_uses ())
> + hazard (set->first_nondebug_insn_use ()->insn ()); // RaW
> +
> + if (!HARD_REGISTER_NUM_P (def->regno ()))
> + continue;
> +
> + // Also check for call clobbers of this def (WaW).
> + for (auto call_group : def->ebb ()->call_clobbers ())
> + {
> + if (!call_group->clobbers (def->resource ()))
> + continue;
> +
> + auto clobber_insn = next_call_clobbers_ignoring (*call_group,
> + def->insn (),
> + no_ignore);
> + if (clobber_insn)
> + hazard (clobber_insn);
> + }
> + }
> +
> + // Find any subsequent defs of uses in INSN (WaR).
> + for (auto use : insn->uses ())
> + {
> + if (ignore_use (use))
> + continue;
> +
> + if (use->def ())
> + {
> + auto def = use->def ()->next_def ();
> + if (def && def->insn () == insn)
> + def = def->next_def ();
> +
> + if (def)
> + hazard (def->insn ());
> + }
> +
> + if (!HARD_REGISTER_NUM_P (use->regno ()))
> + continue;
> +
> + // Also need to handle call clobbers of our uses (again WaR).
> + //
> + // See restrict_movement_for_uses_ignoring for why we don't
> + // need to check backwards for call clobbers.
> + for (auto call_group : use->ebb ()->call_clobbers ())
> + {
> + if (!call_group->clobbers (use->resource ()))
> + continue;
> +
> + auto clobber_insn = next_call_clobbers_ignoring (*call_group,
> + use->insn (),
> + no_ignore);
> + if (clobber_insn)
> + hazard (clobber_insn);
> + }
> + }
> +
> + return result;
> +}
> +
> +// Return true iff R1 and R2 overlap.
> +static bool
> +ranges_overlap_p (const insn_range_info &r1, const insn_range_info &r2)
> +{
> + // If either range is empty, then their intersection is empty.
> + if (!r1 || !r2)
> + return false;
> +
> + // When do they not overlap? When one range finishes before the other
> + // starts, i.e. (*r1.last < *r2.first || *r2.last < *r1.first).
> + // Inverting this, we get the below.
> + return *r1.last >= *r2.first && *r2.last >= *r1.first;
> +}
> +
> +// Get the range of insns that def feeds.
> +static insn_range_info get_def_range (def_info *def)
> +{
> + insn_info *last = def->next_def ()->insn ()->prev_nondebug_insn ();
> + return { def->insn (), last };
> +}
> +
> +// Given a def (of memory), return the downwards range within which we
> +// can safely move this def.
> +static insn_range_info
> +def_downwards_move_range (def_info *def)
> +{
> + auto range = get_def_range (def);
> +
> + auto set = dyn_cast<set_info *> (def);
> + if (!set || !set->has_any_uses ())
> + return range;
> +
> + auto use = set->first_nondebug_insn_use ();
> + if (use)
> + range = move_earlier_than (range, use->insn ());
> +
> + return range;
> +}
> +
> +// Given a def (of memory), return the upwards range within which we can
> +// safely move this def.
> +static insn_range_info
> +def_upwards_move_range (def_info *def)
> +{
> + def_info *prev = def->prev_def ();
> + insn_range_info range { prev->insn (), def->insn () };
> +
> + auto set = dyn_cast<set_info *> (prev);
> + if (!set || !set->has_any_uses ())
> + return range;
> +
> + auto use = set->last_nondebug_insn_use ();
> + if (use)
> + range = move_later_than (range, use->insn ());
> +
> + return range;
> +}
> +
> +// Given candidate store insns FIRST and SECOND, see if we can re-purpose one
> +// of them (together with its def of memory) for the stp/stxvp insn. If so,
> +// return that insn. Otherwise, return null.
> +static insn_info *
> +decide_store_strategy (insn_info *first,
> + insn_info *second,
> + const insn_range_info &move_range)
> +{
> + def_info * const defs[2] = {
> + memory_access (first->defs ()),
> + memory_access (second->defs ())
> + };
> +
> + if (move_range.includes (first)
> + || ranges_overlap_p (move_range, def_downwards_move_range (defs[0])))
> + return first;
> +
> + if (move_range.includes (second)
> + || ranges_overlap_p (move_range, def_upwards_move_range (defs[1])))
> + return second;
> +
> + return nullptr;
> +}
> +
> +// Generate the RTL pattern for a "tombstone"; used temporarily during this
> pass
> +// to replace stores that are marked for deletion where we can't immediately
> +// delete the store (since there are uses of mem hanging off the store).
> +//
> +// These are deleted at the end of the pass and uses re-parented
> appropriately
> +// at this point.
> +static rtx
> +gen_tombstone (void)
> +{
> + return gen_rtx_CLOBBER (VOIDmode,
> + gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode)));
> +}
> +// Go through the reg notes rooted at NOTE, dropping those that we should
> drop,
> +// and preserving those that we want to keep by prepending them to (and
> +// returning) RESULT. EH_REGION is used to make sure we have at most one
> +// REG_EH_REGION note in the resulting list. FR_EXPR is used to return any
> +// REG_FRAME_RELATED_EXPR note we find, as these can need special handling in
> +// combine_reg_notes.
> +static rtx
> +filter_notes (rtx note, rtx result, bool *eh_region, rtx *fr_expr)
> +{
> + for (; note; note = XEXP (note, 1))
> + {
> + switch (REG_NOTE_KIND (note))
> + {
> + case REG_DEAD:
> + // REG_DEAD notes aren't required to be maintained.
> + case REG_EQUAL:
> + case REG_EQUIV:
> + case REG_UNUSED:
> + case REG_NOALIAS:
> + // These can all be dropped. For REG_EQU{AL,IV} they cannot apply to
> + // non-single_set insns, and REG_UNUSED is re-computed by RTl-SSA, see
> + // rtl-ssa/changes.cc:update_notes.
> + //
> + // Similarly, REG_NOALIAS cannot apply to a parallel.
> + case REG_INC:
> + // When we form the pair insn, the reg update is implemented
> + // as just another SET in the parallel, so isn't really an
> + // auto-increment in the RTL sense, hence we drop the note.
> + break;
> + case REG_EH_REGION:
> + gcc_assert (!*eh_region);
> + *eh_region = true;
> + result = alloc_reg_note (REG_EH_REGION, XEXP (note, 0), result);
> + break;
> + case REG_CFA_DEF_CFA:
> + case REG_CFA_OFFSET:
> + case REG_CFA_RESTORE:
> + result = alloc_reg_note (REG_NOTE_KIND (note),
> + copy_rtx (XEXP (note, 0)),
> + result);
> + break;
> + case REG_FRAME_RELATED_EXPR:
> + gcc_assert (!*fr_expr);
> + *fr_expr = copy_rtx (XEXP (note, 0));
> + break;
> + default:
> + // Unexpected REG_NOTE kind.
> + gcc_unreachable ();
> + }
> + }
> +
> + return result;
> +}
> +
> +// Return the notes that should be attached to a combination of I1 and I2,
> where
> +// *I1 < *I2. LOAD_P is true for loads.
> +static rtx
> +combine_reg_notes (insn_info *i1, insn_info *i2, bool load_p)
> +{
> + // Temporary storage for REG_FRAME_RELATED_EXPR notes.
> + rtx fr_expr[2] = {};
> +
> + bool found_eh_region = false;
> + rtx result = NULL_RTX;
> + result = filter_notes (REG_NOTES (i2->rtl ()), result,
> + &found_eh_region, fr_expr);
> + result = filter_notes (REG_NOTES (i1->rtl ()), result,
> + &found_eh_region, fr_expr + 1);
> +
> + if (!load_p)
> + {
> + // Simple frame-related sp-relative saves don't need CFI notes, but
> when
> + // we combine them into an stp/stxvp we will need a CFI note as
> dwarf2cfi
> + // can't interpret the unspec pair representation directly.
> + if (RTX_FRAME_RELATED_P (i1->rtl ()) && !fr_expr[0])
> + fr_expr[0] = copy_rtx (PATTERN (i1->rtl ()));
> + if (RTX_FRAME_RELATED_P (i2->rtl ()) && !fr_expr[1])
> + fr_expr[1] = copy_rtx (PATTERN (i2->rtl ()));
> + }
> +
> + rtx fr_pat = NULL_RTX;
> + if (fr_expr[0] && fr_expr[1])
> + {
> + // Combining two frame-related insns, need to construct
> + // a REG_FRAME_RELATED_EXPR note which represents the combined
> + // operation.
> + RTX_FRAME_RELATED_P (fr_expr[1]) = 1;
> + fr_pat = gen_rtx_PARALLEL (VOIDmode,
> + gen_rtvec (2, fr_expr[0], fr_expr[1]));
> + }
> + else
> + fr_pat = fr_expr[0] ? fr_expr[0] : fr_expr[1];
> +
> + if (fr_pat)
> + result = alloc_reg_note (REG_FRAME_RELATED_EXPR,
> + fr_pat, result);
> +
> + return result;
> +}
> +
> +// Given two memory accesses in PATS, at least one of which is of a
> +// writeback form, extract two non-writeback memory accesses addressed
> +// relative to the initial value of the base register, and output these
> +// in PATS. Return an rtx that represents the overall change to the
> +// base register.
> +static rtx
> +extract_writebacks (bool load_p, rtx pats[2], int changed)
> +{
> + rtx base_reg = NULL_RTX;
> + poly_int64 current_offset = 0;
> +
> + poly_int64 offsets[2];
> +
> + for (int i = 0; i < 2; i++)
> + {
> + rtx mem = XEXP (pats[i], load_p);
> + rtx reg = XEXP (pats[i], !load_p);
> +
> + rtx addr = XEXP (mem, 0);
> + const bool autoinc_p = GET_RTX_CLASS (GET_CODE (addr)) == RTX_AUTOINC;
> +
> + poly_int64 offset;
> + rtx this_base = load_strip_offset (mem, &offset);
> + gcc_assert (REG_P (this_base));
> + if (base_reg)
> + gcc_assert (rtx_equal_p (base_reg, this_base));
> + else
> + base_reg = this_base;
> +
> + // If we changed base for the current insn, then we already
> + // derived the correct mem for this insn from the effective
> + // address of the other access.
> + if (i == changed)
> + {
> + gcc_checking_assert (!autoinc_p);
> + offsets[i] = offset;
> + continue;
> + }
> +
> + if (autoinc_p && any_pre_modify_p (addr))
> + current_offset += offset;
> +
> + poly_int64 this_off = current_offset;
> + if (!autoinc_p)
> + this_off += offset;
> +
> + offsets[i] = this_off;
> + rtx new_mem = change_address (mem, GET_MODE (mem),
> + plus_constant (GET_MODE (base_reg),
> + base_reg, this_off));
> + pats[i] = load_p
> + ? gen_rtx_SET (reg, new_mem)
> + : gen_rtx_SET (new_mem, reg);
> +
> + if (autoinc_p && any_post_modify_p (addr))
> + current_offset += offset;
> + }
> +
> + if (known_eq (current_offset, 0))
> + return NULL_RTX;
> +
> + return gen_rtx_SET (base_reg, plus_constant (GET_MODE (base_reg),
> + base_reg, current_offset));
> +}
> +
> +// We just emitted a tombstone with uid UID, track it in a bitmap for
> +// this BB so we can easily identify it later when cleaning up tombstones.
> +void
> +fusion_bb_info::track_tombstone (int uid)
> +{
> + if (!m_emitted_tombstone)
> + {
> + // Lazily initialize the bitmap for tracking tombstone insns.
> + bitmap_obstack_initialize (&m_bitmap_obstack);
> + bitmap_initialize (&m_tombstone_bitmap, &m_bitmap_obstack);
> + m_emitted_tombstone = true;
> + }
> +
> + if (!bitmap_set_bit (&m_tombstone_bitmap, uid))
> + gcc_unreachable (); // Bit should have changed.
> +}
> +
> +// Virtual base class for load/store walkers used in alias analysis.
> +struct alias_walker
> +{
> + virtual bool conflict_p (int &budget) const = 0;
> + virtual insn_info *insn () const = 0;
> + virtual bool valid () const = 0;
> + virtual void advance () = 0;
> +};
> +
> +// Implement some common functionality used by both store_walker
> +// and load_walker.
> +template<bool reverse>
> +class def_walker : public alias_walker
> +{
> +protected:
> + using def_iter_t = typename std::conditional<reverse,
> + reverse_def_iterator, def_iterator>::type;
> +
> + static use_info *start_use_chain (def_iter_t &def_iter)
> + {
> + set_info *set = nullptr;
> + for (; *def_iter; def_iter++)
> + {
> + set = dyn_cast<set_info *> (*def_iter);
> + if (!set)
> + continue;
> +
> + use_info *use = reverse
> + ? set->last_nondebug_insn_use ()
> + : set->first_nondebug_insn_use ();
> +
> + if (use)
> + return use;
> + }
> +
> + return nullptr;
> + }
> +
> + def_iter_t def_iter;
> + insn_info *limit;
> + def_walker (def_info *def, insn_info *limit) :
> + def_iter (def), limit (limit) {}
> +
> + virtual bool iter_valid () const { return *def_iter; }
> +
> +public:
> + insn_info *insn () const override { return (*def_iter)->insn (); }
> + void advance () override { def_iter++; }
> + bool valid () const override final
> + {
> + if (!iter_valid ())
> + return false;
> +
> + if (reverse)
> + return *(insn ()) > *limit;
> + else
> + return *(insn ()) < *limit;
> + }
> +};
> +
> +// alias_walker that iterates over stores.
> +template<bool reverse, typename InsnPredicate>
> +class store_walker : public def_walker<reverse>
> +{
> + rtx cand_mem;
> + InsnPredicate tombstone_p;
> +
> +public:
> + store_walker (def_info *mem_def, rtx mem, insn_info *limit_insn,
> + InsnPredicate tombstone_fn) :
> + def_walker<reverse> (mem_def, limit_insn),
> + cand_mem (mem), tombstone_p (tombstone_fn) {}
> +
> + bool conflict_p (int &budget) const override final
> + {
> + if (tombstone_p (this->insn ()))
> + return false;
> +
> + return store_modifies_mem_p (cand_mem, this->insn (), budget);
> + }
> +};
> +
> +// alias_walker that iterates over loads.
> +template<bool reverse>
> +class load_walker : public def_walker<reverse>
> +{
> + using Base = def_walker<reverse>;
> + using use_iter_t = typename std::conditional<reverse,
> + reverse_use_iterator, nondebug_insn_use_iterator>::type;
> +
> + use_iter_t use_iter;
> + insn_info *cand_store;
> +
> + bool iter_valid () const override final { return *use_iter; }
> +
> +public:
> + void advance () override final
> + {
> + use_iter++;
> + if (*use_iter)
> + return;
> + this->def_iter++;
> + use_iter = Base::start_use_chain (this->def_iter);
> + }
> +
> + insn_info *insn () const override final
> + {
> + return (*use_iter)->insn ();
> + }
> +
> + bool conflict_p (int &budget) const override final
> + {
> + return load_modified_by_store_p (insn (), cand_store, budget);
> + }
> +
> + load_walker (def_info *def, insn_info *store, insn_info *limit_insn)
> + : Base (def, limit_insn),
> + use_iter (Base::start_use_chain (this->def_iter)),
> + cand_store (store) {}
> +};
> +
> +// Given INSNS (in program order) which are known to be adjacent, look
> +// to see if either insn has a suitable RTL (register) base that we can
> +// use to form a pair. Push these to BASE_CANDS if we find any. CAND_MEMs
> +// gives the relevant mems from the candidate insns, ACCESS_SIZE gives the
> +// size of a single candidate access, and REVERSED says whether the accesses
> +// are inverted in offset order.
> +//
> +// Returns an integer where bit (1 << i) is set if INSNS[i] uses writeback
> +// addressing.
> +static int
> +get_viable_bases (insn_info *insns[2],
> + vec<base_cand> &base_cands,
> + rtx cand_mems[2],
> + unsigned access_size,
> + bool reversed)
> +{
> + // We discovered this pair through a common base. Need to ensure that
> + // we have a common base register that is live at both locations.
> + def_info *base_defs[2] = {};
> + int writeback = 0;
> + for (int i = 0; i < 2; i++)
> + {
> + const bool is_lower = (i == reversed);
> + poly_int64 poly_off;
> + rtx base = load_strip_offset (cand_mems[i], &poly_off);
> + if (GET_RTX_CLASS (GET_CODE (XEXP (cand_mems[i], 0))) == RTX_AUTOINC)
> + writeback |= (1 << i);
> +
> + if (!REG_P (base) || !poly_off.is_constant ())
> + continue;
> +
> + // Punt on accesses relative to eliminable regs. See the comment in
> + // fusion_bb_info::track_access for a detailed explanation of this.
> + if (!reload_completed
> + && (REGNO (base) == FRAME_POINTER_REGNUM
> + || REGNO (base) == ARG_POINTER_REGNUM))
> + continue;
> +
> + HOST_WIDE_INT base_off = poly_off.to_constant ();
> +
> + // It should be unlikely that we ever punt here, since MEM_EXPR offset
> + // alignment should be a good proxy for register offset alignment.
> + if (base_off % access_size != 0)
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "base not viable, offset misaligned (insn %d)\n",
> + insns[i]->uid ());
> + continue;
> + }
> +
> + base_off /= access_size;
> +
> + if (!is_lower)
> + base_off--;
> +
> + if (base_off < LDP_MIN_IMM || base_off > LDP_MAX_IMM)
> + continue;
> +
> + use_info *use = find_access (insns[i]->uses (), REGNO (base));
> + gcc_assert (use);
> + base_defs[i] = use->def ();
> + }
> +
> + if (!base_defs[0] && !base_defs[1])
> + {
> + if (dump_file)
> + fprintf (dump_file, "no viable base register for pair (%d,%d)\n",
> + insns[0]->uid (), insns[1]->uid ());
> + return writeback;
> + }
> +
> + for (int i = 0; i < 2; i++)
> + if ((writeback & (1 << i)) && !base_defs[i])
> + {
> + if (dump_file)
> + fprintf (dump_file, "insn %d has writeback but base isn't viable\n",
> + insns[i]->uid ());
> + return writeback;
> + }
> +
> + if (writeback == 3
> + && base_defs[0]->regno () != base_defs[1]->regno ())
> + {
> + if (dump_file)
> + fprintf (dump_file,
> + "pair (%d,%d): double writeback with distinct regs (%d,%d): "
> + "punting\n",
> + insns[0]->uid (), insns[1]->uid (),
> + base_defs[0]->regno (), base_defs[1]->regno ());
> + return writeback;
> + }
> +
> + if (base_defs[0] && base_defs[1]
> + && base_defs[0]->regno () == base_defs[1]->regno ())
> + {
> + // Easy case: insns already share the same base reg.
> + base_cands.quick_push (base_defs[0]);
> + return writeback;
> + }
> +
> + // Otherwise, we know that one of the bases must change.
> + //
> + // Note that if there is writeback we must use the writeback base
> + // (we know now there is exactly one).
> + for (int i = 0; i < 2; i++)
> + if (base_defs[i] && (!writeback || (writeback & (1 << i))))
> + base_cands.quick_push (base_cand { base_defs[i], i });
> +
> + return writeback;
> +}
> +
> +static void
> +dump_insn_list (FILE *f, const insn_list_t &l)
> +{
> + fprintf (f, "(");
> +
> + auto i = l.begin ();
> + auto end = l.end ();
> +
> + if (i != end)
> + fprintf (f, "%d", (*i)->uid ());
> + i++;
> +
> + for (; i != end; i++)
> + fprintf (f, ", %d", (*i)->uid ());
> +
> + fprintf (f, ")");
> +}
> +
> +DEBUG_FUNCTION void
> +debug (const insn_list_t &l)
> +{
> + dump_insn_list (stderr, l);
> + fprintf (stderr, "\n");
> +}
> +
> +// If we emitted tombstone insns for this BB, iterate through the BB
> +// and remove all the tombstone insns, being sure to reparent any uses
> +// of mem to previous defs when we do this.
> +void
> +fusion_bb_info::cleanup_tombstones ()
> +{
> + // No need to do anything if we didn't emit a tombstone insn for this BB.
> + if (!m_emitted_tombstone)
> + return;
> +
> + insn_info *insn = m_bb->head_insn ();
> + while (insn)
> + {
> + insn_info *next = insn->next_nondebug_insn ();
> + if (!insn->is_real ()
> + || !bitmap_bit_p (&m_tombstone_bitmap, insn->uid ()))
> + {
> + insn = next;
> + continue;
> + }
> +
> + auto def = memory_access (insn->defs ());
> + auto set = dyn_cast<set_info *> (def);
> + if (set && set->has_any_uses ())
> + {
> + def_info *prev_def = def->prev_def ();
> + auto prev_set = dyn_cast<set_info *> (prev_def);
> + if (!prev_set)
> + gcc_unreachable ();
> +
> + while (set->first_use ())
> + crtl->ssa->reparent_use (set->first_use (), prev_set);
> + }
> +
> + // Now set has no uses, we can delete it.
> + insn_change change (insn, insn_change::DELETE);
> + //df_insn_rescan (insn->rtl());
> + crtl->ssa->change_insn (change);
> + insn = next;
> + }
> +}
> +
> +template<typename Map>
> +void
> +fusion_bb_info::traverse_base_map (Map &map)
> +{
> + for (auto kv : map)
> + {
> + const auto &key = kv.first;
> + auto &value = kv.second;
> + transform_for_base (key.second, value);
> + }
> +}
> +
> +void
> +fusion_bb_info::transform ()
> +{
> + traverse_base_map (expr_map);
> + traverse_base_map (def_map);
> +}
> diff --git a/gcc/testsuite/g++.target/powerpc/vecload.C
> b/gcc/testsuite/g++.target/powerpc/vecload.C
> new file mode 100644
> index 00000000000..c523572cf3c
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/vecload.C
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
> +
> +#include <altivec.h>
> +
> +void
> +foo (__vector_quad *dst, vector unsigned char *ptr, vector unsigned char src)
> +{
> + __vector_quad acc;
> + __builtin_mma_xvf32ger(&acc, src, ptr[0]);
> + __builtin_mma_xvf32gerpp(&acc, src, ptr[1]);
> + *dst = acc;
> +}
> +/* { dg-final { scan-assembler {\mlxvp\M} } } */
> diff --git a/gcc/testsuite/g++.target/powerpc/vecload1.C
> b/gcc/testsuite/g++.target/powerpc/vecload1.C
> new file mode 100644
> index 00000000000..d10ff0cdf36
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/vecload1.C
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
> +
> +#include <altivec.h>
> +
> +void
> +foo2 ()
> +{
> + __vector_quad *dst1;
> + __vector_quad *dst2;
> + vector unsigned char src;
> + __vector_quad acc;
> + vector unsigned char *ptr;
> + __builtin_mma_xvf32ger(&acc, src, ptr[0]);
> + __builtin_mma_xvf32gerpp(&acc, src, ptr[1]);
> + *dst1 = acc;
> + __builtin_mma_xvf32ger(&acc, src, ptr[2]);
> + __builtin_mma_xvf32gerpp(&acc, src, ptr[3]);
> + *dst2 = acc;
> +}
> +/* { dg-final { scan-assembler {\mlxvp\M} } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/mma-builtin-1.c
> b/gcc/testsuite/gcc.target/powerpc/mma-builtin-1.c
> index 69ee826e1be..ae29127f954 100644
> --- a/gcc/testsuite/gcc.target/powerpc/mma-builtin-1.c
> +++ b/gcc/testsuite/gcc.target/powerpc/mma-builtin-1.c
> @@ -258,8 +258,8 @@ foo13b (__vector_quad *dst, __vector_quad *src, vec_t
> *vec)
> dst[13] = acc;
> }
>
> -/* { dg-final { scan-assembler-times {\mlxv\M} 40 } } */
> -/* { dg-final { scan-assembler-times {\mlxvp\M} 12 } } */
> +/* { dg-final { scan-assembler-times {\mlxv\M} 0 } } */
> +/* { dg-final { scan-assembler-times {\mlxvp\M} 32 } } */
> /* { dg-final { scan-assembler-times {\mstxvp\M} 40 } } */
> /* { dg-final { scan-assembler-times {\mxxmfacc\M} 20 } } */
> /* { dg-final { scan-assembler-times {\mxxmtacc\M} 6 } } */
> --
> 2.39.3
>
