Jennifer Schmitz <jschm...@nvidia.com> writes:
> Thanks for the feedback! I updated the patch based on your comments, more
> detailed comments inline below. The updated version was bootstrapped and
> tested again, no regression.
> Best,
> Jennifer
>
> From 89936b7bc2de7a1e4bc55c3a1e8d5e6ac0de579d Mon Sep 17 00:00:00 2001
> From: Jennifer Schmitz <jschm...@nvidia.com>
> Date: Wed, 24 Jul 2024 06:13:59 -0700
> Subject: [PATCH] AArch64: Fuse CMP+CSEL and CMP+CSET for -mcpu=neoverse-v2
>
> According to the Neoverse V2 Software Optimization Guide (section 4.14), the
> instruction pairs CMP+CSEL and CMP+CSET can be fused, which had not been
> implemented so far. This patch implements and tests the two fusion pairs.
>
> The patch was bootstrapped and regtested on aarch64-linux-gnu, no regression.
> There was also no non-noise impact on SPEC CPU2017 benchmark.
> OK for mainline?
>
> Signed-off-by: Jennifer Schmitz <jschm...@nvidia.com>
>
> gcc/
>
> * config/aarch64/aarch64.cc (aarch_macro_fusion_pair_p): Implement
> fusion logic.
> * config/aarch64/aarch64-fusion-pairs.def (cmp+csel): New entry.
> (cmp+cset): Likewise.
> * config/aarch64/tuning_models/neoversev2.h: Enable logic in
> field fusible_ops.
>
> gcc/testsuite/
>
> * gcc.target/aarch64/fuse_cmp_csel.c: New test.
> * gcc.target/aarch64/fuse_cmp_cset.c: Likewise.
> ---
> gcc/config/aarch64/aarch64-fusion-pairs.def | 2 ++
> gcc/config/aarch64/aarch64.cc | 20 +++++++++++
> gcc/config/aarch64/tuning_models/neoversev2.h | 5 ++-
> .../gcc.target/aarch64/fuse_cmp_csel.c | 33 +++++++++++++++++++
> .../gcc.target/aarch64/fuse_cmp_cset.c | 31 +++++++++++++++++
> 5 files changed, 90 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/fuse_cmp_csel.c
> create mode 100644 gcc/testsuite/gcc.target/aarch64/fuse_cmp_cset.c
>
> diff --git a/gcc/config/aarch64/aarch64-fusion-pairs.def
> b/gcc/config/aarch64/aarch64-fusion-pairs.def
> index 9a43b0c8065..bf5e85ba8fe 100644
> --- a/gcc/config/aarch64/aarch64-fusion-pairs.def
> +++ b/gcc/config/aarch64/aarch64-fusion-pairs.def
> @@ -37,5 +37,7 @@ AARCH64_FUSION_PAIR ("aes+aesmc", AES_AESMC)
> AARCH64_FUSION_PAIR ("alu+branch", ALU_BRANCH)
> AARCH64_FUSION_PAIR ("alu+cbz", ALU_CBZ)
> AARCH64_FUSION_PAIR ("addsub_2reg_const1", ADDSUB_2REG_CONST1)
> +AARCH64_FUSION_PAIR ("cmp+csel", CMP_CSEL)
> +AARCH64_FUSION_PAIR ("cmp+cset", CMP_CSET)
>
> #undef AARCH64_FUSION_PAIR
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index e0cf382998c..d42c153443e 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -27345,6 +27345,26 @@ aarch_macro_fusion_pair_p (rtx_insn *prev, rtx_insn
> *curr)
> && reg_referenced_p (SET_DEST (prev_set), PATTERN (curr)))
> return true;
>
> + /* Fuse CMP and CSEL/CSET. */
> + if (prev_set && curr_set
> + && GET_CODE (SET_SRC (prev_set)) == COMPARE
> + && SCALAR_INT_MODE_P (GET_MODE (XEXP (SET_SRC (prev_set), 0)))
> + && reg_referenced_p (SET_DEST (prev_set), PATTERN (curr)))
> + {
> + enum attr_type prev_type = get_attr_type (prev);
> + if ((prev_type == TYPE_ALUS_SREG || prev_type == TYPE_ALUS_IMM)
> + && (aarch64_fusion_enabled_p (AARCH64_FUSE_CMP_CSEL)
> + && GET_CODE (SET_SRC (curr_set)) == IF_THEN_ELSE
> + && REG_P (XEXP (SET_SRC (curr_set), 1))
> + && REG_P (XEXP (SET_SRC (curr_set), 2))
Gah, I'd meant to say this in a previous review, but now realise
that I forgot. I think these REG_Ps can be relaxed to:
&& aarch64_reg_or_zero (XEXP (SET_SRC (curr_set), N), VOIDmode)
since CSEL can take w/xzr.
> + && SCALAR_INT_MODE_P (GET_MODE (XEXP (SET_SRC (curr_set), 1))))
> + || (aarch64_fusion_enabled_p (AARCH64_FUSE_CMP_CSET)
> + && GET_RTX_CLASS (GET_CODE (SET_SRC (curr_set)))
> + == RTX_COMPARE
> + && REG_P (SET_DEST (curr_set))))
> + return true;
> + }
It looks like there might be a missing pair of brackets here. AFAICT,
the current bracketing works out as:
if ((prev_type conditions)
&& (CMP_CSEL conditions)
|| (CMP_CSET conditions))
and since || binds less tightly than &&, I think the CMP_CSET condition
doesn't include the prev_type restriction. Enclosing everything after
the first && in an extra set of brackets would fix that.
OK with those changes from my POV (no need for another round of review,
unless you'd prefer one). Please give others a day or so to comment though.
Thanks,
Richard
> +
> /* Fuse flag-setting ALU instructions and conditional branch. */
> if (aarch64_fusion_enabled_p (AARCH64_FUSE_ALU_BRANCH)
> && any_condjump_p (curr))
> diff --git a/gcc/config/aarch64/tuning_models/neoversev2.h
> b/gcc/config/aarch64/tuning_models/neoversev2.h
> index f76e4ef358f..ae99fab22d8 100644
> --- a/gcc/config/aarch64/tuning_models/neoversev2.h
> +++ b/gcc/config/aarch64/tuning_models/neoversev2.h
> @@ -221,7 +221,10 @@ static const struct tune_params neoversev2_tunings =
> 2 /* store_pred. */
> }, /* memmov_cost. */
> 5, /* issue_rate */
> - (AARCH64_FUSE_AES_AESMC | AARCH64_FUSE_CMP_BRANCH), /* fusible_ops */
> + (AARCH64_FUSE_AES_AESMC
> + | AARCH64_FUSE_CMP_BRANCH
> + | AARCH64_FUSE_CMP_CSEL
> + | AARCH64_FUSE_CMP_CSET), /* fusible_ops */
> "32:16", /* function_align. */
> "4", /* jump_align. */
> "32:16", /* loop_align. */
> diff --git a/gcc/testsuite/gcc.target/aarch64/fuse_cmp_csel.c
> b/gcc/testsuite/gcc.target/aarch64/fuse_cmp_csel.c
> new file mode 100644
> index 00000000000..85f302bab98
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/fuse_cmp_csel.c
> @@ -0,0 +1,33 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mcpu=neoverse-v2" } */
> +/* { dg-final { check-function-bodies "**" "" } } */
> +
> +/*
> +** f1:
> +** ...
> +** cmp w[0-9]+, w[0-9]+
> +** csel w[0-9]+, w[0-9]+, w[0-9]+, le
> +** ret
> +*/
> +int f1 (int a, int b, int c)
> +{
> + int cmp = a > b;
> + int add1 = c + 3;
> + int add2 = c + 8;
> + return cmp ? add1 : add2;
> +}
> +
> +/*
> +** f2:
> +** ...
> +** cmp x[0-9]+, x[0-9]+
> +** csel x[0-9]+, x[0-9]+, x[0-9]+, le
> +** ret
> +*/
> +long long f2 (long long a, long long b, long long c)
> +{
> + long long cmp = a > b;
> + long long add1 = c + 3;
> + long long add2 = c + 8;
> + return cmp ? add1 : add2;
> +}
> diff --git a/gcc/testsuite/gcc.target/aarch64/fuse_cmp_cset.c
> b/gcc/testsuite/gcc.target/aarch64/fuse_cmp_cset.c
> new file mode 100644
> index 00000000000..04f1ce2773b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/fuse_cmp_cset.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mcpu=neoverse-v2" } */
> +/* { dg-final { check-function-bodies "**" "" } } */
> +
> +/*
> +** f1:
> +** cmp w[0-9]+, w[0-9]+
> +** cset w[0-9]+, gt
> +** ...
> +*/
> +int g;
> +int f1 (int a, int b)
> +{
> + int cmp = a > b;
> + g = cmp + 1;
> + return cmp;
> +}
> +
> +/*
> +** f2:
> +** cmp x[0-9]+, x[0-9]+
> +** cset x[0-9]+, gt
> +** ...
> +*/
> +long long h;
> +long long f2 (long long a, long long b)
> +{
> + long long cmp = a > b;
> + h = cmp + 1;
> + return cmp;
> +}
