On Wed, 28 Dec 2022 at 19:18, Raphael Moreira Zinsly <
rzin...@ventanamicro.com> wrote:
> The Zbb min/max pattern was not matching 32-bit sources when
> compiling for 64-bit.
> This patch separates the pattern into SImode and DImode, and
> use a define_expand to handle SImode on 64-bit.
> zbb-min-max-02.c generates different code as a result of the new
> expander. The resulting code is as efficient as the old code.
> Furthermore, the special sh1add pattern that appeared in
> zbb-min-max-02.c is tested by the zba-shNadd-* tests.
>
> gcc/ChangeLog:
>
> * config/riscv/bitmanip.md
> (<bitmanip_optab><mode>3): Divide pattern into
> <bitmanip_optab>si3_insn and <bitmanip_optab>di3.
> (<bitmanip_optab>si3): Handle SImode sources on
> TARGET_64BIT.
>
> gcc/testsuite:
>
> * gcc.target/riscv/zbb-abs.c: New test.
> * gcc.target/riscv/zbb-min-max-02.c: Addapt the
> expected output.
> ---
> gcc/config/riscv/bitmanip.md | 38 ++++++++++++++++---
> gcc/testsuite/gcc.target/riscv/zbb-abs.c | 18 +++++++++
> .../gcc.target/riscv/zbb-min-max-02.c | 2 +-
> 3 files changed, 52 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-abs.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index d17133d58c1..abf08a29e89 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -360,14 +360,42 @@
> DONE;
> })
>
> -(define_insn "<bitmanip_optab><mode>3"
> - [(set (match_operand:X 0 "register_operand" "=r")
> - (bitmanip_minmax:X (match_operand:X 1 "register_operand" "r")
> - (match_operand:X 2 "register_operand" "r")))]
> - "TARGET_ZBB"
> +(define_insn "<bitmanip_optab>si3_insn"
> + [(set (match_operand:SI 0 "register_operand" "=r")
> + (bitmanip_minmax:SI (match_operand:SI 1 "register_operand" "r")
> + (match_operand:SI 2 "register_operand" "r")))]
> + "!TARGET_64BIT && TARGET_ZBB"
> "<bitmanip_insn>\t%0,%1,%2"
> [(set_attr "type" "bitmanip")])
>
> +(define_insn "<bitmanip_optab>di3"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (bitmanip_minmax:DI (match_operand:DI 1 "register_operand" "r")
> + (match_operand:DI 2 "register_operand" "r")))]
> + "TARGET_64BIT && TARGET_ZBB"
> + "<bitmanip_insn>\t%0,%1,%2"
> + [(set_attr "type" "bitmanip")])
> +
> +(define_expand "<bitmanip_optab>si3"
> + [(set (match_operand:SI 0 "register_operand" "=r")
> + (bitmanip_minmax:SI (match_operand:SI 1 "register_operand" "r")
> + (match_operand:SI 2 "register_operand" "r")))]
> + "TARGET_ZBB"
> + "
> +{
> + if (TARGET_64BIT)
> + {
> + rtx op1_x = gen_reg_rtx (DImode);
> + emit_move_insn (op1_x, gen_rtx_SIGN_EXTEND (DImode, operands[1]));
> + rtx op2_x = gen_reg_rtx (DImode);
> + emit_move_insn (op2_x, gen_rtx_SIGN_EXTEND (DImode, operands[2]));
> + rtx dst_x = gen_reg_rtx (DImode);
> + emit_insn (gen_<bitmanip_optab>di3 (dst_x, op1_x, op2_x));
> + emit_move_insn (operands[0], gen_lowpart (SImode, dst_x));
> + DONE;
> + }
> +}")
>
We have two issues around min/max here:
1. That it doesn't apply to the SImode abs case (which is due to
expand_abs_nojump() blindly testing for the current mode in smax_optab).
2. That we have to reduce the number of extensions to the least amount.
The above addresses expand_abs_nojump(), but makes the general solution
harder as the middle-end needs to know there is no native SImode min/max
available.
We still plan (proof-of-concept works, but a final patch will likely not be
ready before very late in January) to submit a patch to improve the
expansion of MIN_EXPR/MAX_EXPR that utilizes the type-precision and
value-ranges to not even create the sign-extensions in the first place. If
we do the above, the middle-end will blindly emit this sequence with the 2
sign-extensions — which may or may not be eliminated later by combining
with a w-form.
I'll also add an enhancement to expand_abs_nojump() to our list of changes
for the min/max enhancement during the lowering.
Note that, if we decide to go ahead with using this as a temporary solution
until our change is ready, you'll also need to add a cost for the SImode
max.
Philipp.
> +
> ;; Optimize the common case of a SImode min/max against a constant
> ;; that is safe both for sign- and zero-extension.
> (define_insn_and_split "*minmax"
> diff --git a/gcc/testsuite/gcc.target/riscv/zbb-abs.c
> b/gcc/testsuite/gcc.target/riscv/zbb-abs.c
> new file mode 100644
> index 00000000000..6ef7efdbd49
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/zbb-abs.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc_zbb" } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" } } */
> +
> +#define ABS(x) (((x) >= 0) ? (x) : -(x))
> +
> +int
> +foo (int x)
> +{
> + return ABS(x);
> +}
> +
> +/* { dg-final { scan-assembler-times "neg" 1 } } */
> +/* { dg-final { scan-assembler-times "max" 1 } } */
> +/* { dg-final { scan-assembler-not "sraiw" } } */
> +/* { dg-final { scan-assembler-not "xor" } } */
> +/* { dg-final { scan-assembler-not "subw" } } */
> +
> diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-02.c
> b/gcc/testsuite/gcc.target/riscv/zbb-min-max-02.c
> index b462859f10f..b9db655d55d 100644
> --- a/gcc/testsuite/gcc.target/riscv/zbb-min-max-02.c
> +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-02.c
> @@ -9,6 +9,6 @@ int f(unsigned int* a)
> }
>
> /* { dg-final { scan-assembler-times "minu" 1 } } */
> -/* { dg-final { scan-assembler-times "sext.w" 1 } } */
> +/* { dg-final { scan-assembler-times "sext.w|addw" 1 } } */
> /* { dg-final { scan-assembler-not "zext.w" } } */
>
> --
> 2.38.1
>
>
