[PATCH] RISC-V: Add min/max patterns for ifcvt.

[PATCH] RISC-V: Add min/max patterns for ifcvt.

[Robin Dapp]

Hi,

ifcvt likes to emit

(set
  (if_then_else)
    (ge (reg 1) (reg2))
    (reg 1)
    (reg 2))

which can be recognized as min/max patterns in the backend.
This patch adds such patterns and the respective iterators as well as a
test.

This depends on the generic ifcvt change.
Regtested on rv64gcv_zvfh_zicond_zbb_zvbb. 

Regards
 Robin

gcc/ChangeLog:

	* config/riscv/bitmanip.md (*<bitmanip_minmax_cmp_insn>_cmp_<mode>3):
	New min/max ifcvt pattern.
	* config/riscv/iterators.md (minu): New iterator.
	* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
	Remove riscv-specific adjustment.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/zbb-min-max-04.c: New test.
---
 gcc/config/riscv/bitmanip.md                  | 13 +++++
 gcc/config/riscv/iterators.md                 |  8 ++++
 gcc/config/riscv/riscv.cc                     |  3 --
 .../gcc.target/riscv/zbb-min-max-04.c         | 47 +++++++++++++++++++
 4 files changed, 68 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c

diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
index 8769a6b818b..11102985796 100644
--- a/gcc/config/riscv/bitmanip.md
+++ b/gcc/config/riscv/bitmanip.md
@@ -547,6 +547,19 @@ (define_insn "*<bitmanip_optab><mode>3"
   "<bitmanip_insn>\t%0,%1,%z2"
   [(set_attr "type" "<bitmanip_insn>")])
 
+;; Provide a minmax pattern for ifcvt to match.
+(define_insn "*<bitmanip_minmax_cmp_insn>_cmp_<mode>3"
+  [(set (match_operand:X 0 "register_operand" "=r")
+	(if_then_else:X
+	    (bitmanip_minmax_cmp_op
+		(match_operand:X 1 "register_operand" "r")
+		(match_operand:X 2 "register_operand" "r"))
+	    (match_dup 1)
+	    (match_dup 2)))]
+  "TARGET_ZBB"
+  "<bitmanip_minmax_cmp_insn>\t%0,%1,%z2"
+  [(set_attr "type" "<bitmanip_minmax_cmp_insn>")])
+
 ;; 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/config/riscv/iterators.md b/gcc/config/riscv/iterators.md
index 8a9d1986b4a..2f7be6e83c1 100644
--- a/gcc/config/riscv/iterators.md
+++ b/gcc/config/riscv/iterators.md
@@ -202,6 +202,14 @@ (define_code_iterator bitmanip_bitwise [and ior])
 
 (define_code_iterator bitmanip_minmax [smin umin smax umax])
 
+(define_code_iterator bitmanip_minmax_cmp_op [lt ltu le leu ge geu gt gtu])
+
+; Map a comparison operator to a min or max.
+(define_code_attr bitmanip_minmax_cmp_insn [(lt "min") (ltu "minu")
+					    (le "min") (leu "minu")
+					    (ge "max") (geu "maxu")
+					    (gt "max") (gtu "maxu")])
+
 (define_code_iterator clz_ctz_pcnt [clz ctz popcount])
 
 (define_code_iterator bitmanip_rotate [rotate rotatert])
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 13cd61a4a22..d17c0a260a2 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4009,9 +4009,6 @@ riscv_noce_conversion_profitable_p (rtx_insn *seq,
 {
   struct noce_if_info riscv_if_info = *if_info;
 
-  riscv_if_info.original_cost -= COSTS_N_INSNS (2);
-  riscv_if_info.original_cost += insn_cost (if_info->jump, if_info->speed_p);
-
   /* Hack alert!  When `noce_try_store_flag_mask' uses `cstore<mode>4'
      to emit a conditional set operation on DImode output it comes up
      with a sequence such as:
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c
new file mode 100644
index 00000000000..ebf1889075d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c
@@ -0,0 +1,47 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=rv64gc_zicond_zbb -mabi=lp64d" } */
+/* { dg-skip-if "" { *-*-* } { "-finline-functions" "-funroll-loops" "-ftracer" } } */
+
+typedef int move_s;
+
+int
+remove_one_fast (int *move_ordering, const int num_moves, int mark)
+{
+  int i, best = -1000000;
+  int tmp = 0;
+
+  for (i = mark; i < num_moves; i++)
+    {
+      if (move_ordering[i] > best)
+        {
+          best = move_ordering[i];
+          tmp = i;
+        }
+    }
+
+  return tmp;
+}
+
+/* { dg-final { scan-assembler-times "max\t" 1 } }  */
+/* { dg-final { scan-assembler-times "czero.nez" 2 } }  */
+/* { dg-final { scan-assembler-times "czero.eqz" 2 } }  */
+
+int
+remove_one_fast2 (int *move_ordering, const int num_moves, int mark)
+{
+  int i, best = -1000000;
+  int tmp = 0;
+
+  for (i = mark; i < num_moves; i++)
+    {
+      if (move_ordering[i] < best)
+        {
+          best = move_ordering[i];
+          tmp = i;
+        }
+    }
+
+  return tmp;
+}
+
+/* { dg-final { scan-assembler-times "min\t" 1 } }  */
-- 
2.45.1

Re: [PATCH] RISC-V: Add min/max patterns for ifcvt.

[Jeff Law]

On 5/31/24 9:07 AM, Robin Dapp wrote:
> Hi,
> 
> ifcvt likes to emit
> 
> (set
>    (if_then_else)
>      (ge (reg 1) (reg2))
>      (reg 1)
>      (reg 2))
> 
> which can be recognized as min/max patterns in the backend.
> This patch adds such patterns and the respective iterators as well as a
> test.
> 
> This depends on the generic ifcvt change.
> Regtested on rv64gcv_zvfh_zicond_zbb_zvbb.
> 
> Regards
>   Robin
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/bitmanip.md (*<bitmanip_minmax_cmp_insn>_cmp_<mode>3):
> 	New min/max ifcvt pattern.
> 	* config/riscv/iterators.md (minu): New iterator.
> 	* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
> 	Remove riscv-specific adjustment.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/zbb-min-max-04.c: New test.
OK once prereqs are ACK'd.


Presumably this fixes that deepsjeng missed if-conversion we kicked 
around a couple months back?


jeff

Re: [PATCH] RISC-V: Add min/max patterns for ifcvt.

[Palmer Dabbelt]

On Fri, 31 May 2024 08:07:11 PDT (-0700), Robin Dapp wrote:
> Hi,
>
> ifcvt likes to emit
>
> (set
>   (if_then_else)
>     (ge (reg 1) (reg2))
>     (reg 1)
>     (reg 2))
>
> which can be recognized as min/max patterns in the backend.
> This patch adds such patterns and the respective iterators as well as a
> test.
>
> This depends on the generic ifcvt change.

https://inbox.sourceware.org/gcc-patches/57bb6ce5-79c3-4b08-b524-e807b9ac431b@gmail.com/T/#u

in case anyone's looking for it.

> Regtested on rv64gcv_zvfh_zicond_zbb_zvbb.
>
> Regards
>  Robin
>
> gcc/ChangeLog:
>
> 	* config/riscv/bitmanip.md (*<bitmanip_minmax_cmp_insn>_cmp_<mode>3):
> 	New min/max ifcvt pattern.
> 	* config/riscv/iterators.md (minu): New iterator.
> 	* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
> 	Remove riscv-specific adjustment.
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/riscv/zbb-min-max-04.c: New test.
> ---
>  gcc/config/riscv/bitmanip.md                  | 13 +++++
>  gcc/config/riscv/iterators.md                 |  8 ++++
>  gcc/config/riscv/riscv.cc                     |  3 --
>  .../gcc.target/riscv/zbb-min-max-04.c         | 47 +++++++++++++++++++
>  4 files changed, 68 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c
>
> diff --git a/gcc/config/riscv/bitmanip.md b/gcc/config/riscv/bitmanip.md
> index 8769a6b818b..11102985796 100644
> --- a/gcc/config/riscv/bitmanip.md
> +++ b/gcc/config/riscv/bitmanip.md
> @@ -547,6 +547,19 @@ (define_insn "*<bitmanip_optab><mode>3"
>    "<bitmanip_insn>\t%0,%1,%z2"
>    [(set_attr "type" "<bitmanip_insn>")])
>
> +;; Provide a minmax pattern for ifcvt to match.
> +(define_insn "*<bitmanip_minmax_cmp_insn>_cmp_<mode>3"
> +  [(set (match_operand:X 0 "register_operand" "=r")
> +	(if_then_else:X
> +	    (bitmanip_minmax_cmp_op
> +		(match_operand:X 1 "register_operand" "r")
> +		(match_operand:X 2 "register_operand" "r"))
> +	    (match_dup 1)
> +	    (match_dup 2)))]
> +  "TARGET_ZBB"
> +  "<bitmanip_minmax_cmp_insn>\t%0,%1,%z2"
> +  [(set_attr "type" "<bitmanip_minmax_cmp_insn>")])

This is a bit different than how we're handling the other min/max type 
attributes

    (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 "reg_or_0_operand" "rJ")))]
      "TARGET_ZBB"
      "<bitmanip_insn>\t%0,%1,%z2"
      [(set_attr "type" "<bitmanip_insn>")])

but it looks like it ends up with the same types after all the iterators 
(there's some "max vs smax" and "smax vs maxs" juggling, but IIUC it 
ends up in the same place).  I think it'd be clunkier to try and use all 
the same iterators, though, so

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

[I was wondering if we need the reversed, Jeff on the call says we 
don't.  I couldn't figure out how to write a test for it.]

> +
>  ;; 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/config/riscv/iterators.md b/gcc/config/riscv/iterators.md
> index 8a9d1986b4a..2f7be6e83c1 100644
> --- a/gcc/config/riscv/iterators.md
> +++ b/gcc/config/riscv/iterators.md
> @@ -202,6 +202,14 @@ (define_code_iterator bitmanip_bitwise [and ior])
>
>  (define_code_iterator bitmanip_minmax [smin umin smax umax])
>
> +(define_code_iterator bitmanip_minmax_cmp_op [lt ltu le leu ge geu gt gtu])
> +
> +; Map a comparison operator to a min or max.
> +(define_code_attr bitmanip_minmax_cmp_insn [(lt "min") (ltu "minu")
> +					    (le "min") (leu "minu")
> +					    (ge "max") (geu "maxu")
> +					    (gt "max") (gtu "maxu")])
> +
>  (define_code_iterator clz_ctz_pcnt [clz ctz popcount])
>
>  (define_code_iterator bitmanip_rotate [rotate rotatert])
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 13cd61a4a22..d17c0a260a2 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -4009,9 +4009,6 @@ riscv_noce_conversion_profitable_p (rtx_insn *seq,
>  {
>    struct noce_if_info riscv_if_info = *if_info;
>
> -  riscv_if_info.original_cost -= COSTS_N_INSNS (2);
> -  riscv_if_info.original_cost += insn_cost (if_info->jump, if_info->speed_p);
> -
>    /* Hack alert!  When `noce_try_store_flag_mask' uses `cstore<mode>4'
>       to emit a conditional set operation on DImode output it comes up
>       with a sequence such as:
> diff --git a/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c
> new file mode 100644
> index 00000000000..ebf1889075d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/zbb-min-max-04.c
> @@ -0,0 +1,47 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=rv64gc_zicond_zbb -mabi=lp64d" } */
> +/* { dg-skip-if "" { *-*-* } { "-finline-functions" "-funroll-loops" "-ftracer" } } */
> +
> +typedef int move_s;

IIUC that typedef isn't used?

> +
> +int
> +remove_one_fast (int *move_ordering, const int num_moves, int mark)
> +{
> +  int i, best = -1000000;
> +  int tmp = 0;
> +
> +  for (i = mark; i < num_moves; i++)
> +    {
> +      if (move_ordering[i] > best)
> +        {
> +          best = move_ordering[i];
> +          tmp = i;
> +        }
> +    }
> +
> +  return tmp;
> +}
> +
> +/* { dg-final { scan-assembler-times "max\t" 1 } }  */
> +/* { dg-final { scan-assembler-times "czero.nez" 2 } }  */
> +/* { dg-final { scan-assembler-times "czero.eqz" 2 } }  */
> +
> +int
> +remove_one_fast2 (int *move_ordering, const int num_moves, int mark)
> +{
> +  int i, best = -1000000;
> +  int tmp = 0;
> +
> +  for (i = mark; i < num_moves; i++)
> +    {
> +      if (move_ordering[i] < best)
> +        {
> +          best = move_ordering[i];
> +          tmp = i;
> +        }
> +    }
> +
> +  return tmp;
> +}
> +
> +/* { dg-final { scan-assembler-times "min\t" 1 } }  */

Re: [PATCH] RISC-V: Add min/max patterns for ifcvt.

[Jeff Law]

On 6/3/24 11:03 AM, Palmer Dabbelt wrote:

>>
>> +;; Provide a minmax pattern for ifcvt to match.
>> +(define_insn "*<bitmanip_minmax_cmp_insn>_cmp_<mode>3"
>> +  [(set (match_operand:X 0 "register_operand" "=r")
>> +    (if_then_else:X
>> +        (bitmanip_minmax_cmp_op
>> +        (match_operand:X 1 "register_operand" "r")
>> +        (match_operand:X 2 "register_operand" "r"))
>> +        (match_dup 1)
>> +        (match_dup 2)))]
>> +  "TARGET_ZBB"
>> +  "<bitmanip_minmax_cmp_insn>\t%0,%1,%z2"
>> +  [(set_attr "type" "<bitmanip_minmax_cmp_insn>")])
> 
> This is a bit different than how we're handling the other min/max type 
> attributes
> 
>     (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 "reg_or_0_operand" "rJ")))]
>       "TARGET_ZBB"
>       "<bitmanip_insn>\t%0,%1,%z2"
>       [(set_attr "type" "<bitmanip_insn>")])
> 
> but it looks like it ends up with the same types after all the iterators 
> (there's some "max vs smax" and "smax vs maxs" juggling, but IIUC it 
> ends up in the same place).  I think it'd be clunkier to try and use all 
> the same iterators, though, so
> 
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> [I was wondering if we need the reversed, Jeff on the call says we 
> don't.  I couldn't figure out how to write a test for it.]
Right.  I had managed to convince myself that we didn't need the 
reversed case.  I'm less sure now than I was earlier, but I'm also 
confident that if the need arises we can trivially handle it.  At some 
point there's canonicalization of the condition and that's almost 
certainly what's making it hard to synthesize a testcase for the 
reversed pattern.

The other thing I pondered was whether or not we should support SImode 
min/max on rv64.  It was critical for simplifying that abs2 routine in 
x264, but I couldn't convince myself it was needed here.  So I just set 
it aside and didn't mention it.

jeff

Re: [PATCH] RISC-V: Add min/max patterns for ifcvt.

[Palmer Dabbelt]

On Mon, 03 Jun 2024 11:50:54 PDT (-0700), jeffreyalaw@gmail.com wrote:
>
>
> On 6/3/24 11:03 AM, Palmer Dabbelt wrote:
>
>>>
>>> +;; Provide a minmax pattern for ifcvt to match.
>>> +(define_insn "*<bitmanip_minmax_cmp_insn>_cmp_<mode>3"
>>> +  [(set (match_operand:X 0 "register_operand" "=r")
>>> +    (if_then_else:X
>>> +        (bitmanip_minmax_cmp_op
>>> +        (match_operand:X 1 "register_operand" "r")
>>> +        (match_operand:X 2 "register_operand" "r"))
>>> +        (match_dup 1)
>>> +        (match_dup 2)))]
>>> +  "TARGET_ZBB"
>>> +  "<bitmanip_minmax_cmp_insn>\t%0,%1,%z2"
>>> +  [(set_attr "type" "<bitmanip_minmax_cmp_insn>")])
>>
>> This is a bit different than how we're handling the other min/max type
>> attributes
>>
>>     (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 "reg_or_0_operand" "rJ")))]
>>       "TARGET_ZBB"
>>       "<bitmanip_insn>\t%0,%1,%z2"
>>       [(set_attr "type" "<bitmanip_insn>")])
>>
>> but it looks like it ends up with the same types after all the iterators
>> (there's some "max vs smax" and "smax vs maxs" juggling, but IIUC it
>> ends up in the same place).  I think it'd be clunkier to try and use all
>> the same iterators, though, so
>>
>> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> [I was wondering if we need the reversed, Jeff on the call says we
>> don't.  I couldn't figure out how to write a test for it.]
> Right.  I had managed to convince myself that we didn't need the
> reversed case.  I'm less sure now than I was earlier, but I'm also
> confident that if the need arises we can trivially handle it.  At some
> point there's canonicalization of the condition and that's almost
> certainly what's making it hard to synthesize a testcase for the
> reversed pattern.

Ya, no reason to block merging this on the reversed cases.  I just 
couldn't figure out if we should add them.

> The other thing I pondered was whether or not we should support SImode
> min/max on rv64.  It was critical for simplifying that abs2 routine in
> x264, but I couldn't convince myself it was needed here.  So I just set
> it aside and didn't mention it.

I think because we only have the DI compares that we wouldn't need the 
smaller modes?  Maybe we should add them, though, as the hueristics 
around DImode-ifying compares are fragile so we might trip over some 
long-tail performance issues that would be wacky for users to reason 
about.

Either way, also doesn't seem like a reason to block this one.

>
> jeff