[2/5,RISC-V] Generate Zicond instruction for basic semantics

  This patch completes the recognition of the basic semantics
defined in the spec, namely:

Conditional zero, if condition is equal to zero
  rd = (rs2 == 0) ? 0 : rs1
Conditional zero, if condition is non zero
  rd = (rs2 != 0) ? 0 : rs1

gcc/ChangeLog:

	* config/riscv/riscv.md: Include zicond.md
	* config/riscv/zicond.md: New file.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/zicond-primitiveSemantics.c: New test.
---
 gcc/config/riscv/riscv.md                     |  1 +
 gcc/config/riscv/zicond.md                    | 84 +++++++++++++++++++
 .../riscv/zicond-primitiveSemantics.c         | 49 +++++++++++
 3 files changed, 134 insertions(+)
 create mode 100644 gcc/config/riscv/zicond.md
 create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
  

On 7/19/23 04:11, Xiao Zeng wrote:
> This patch completes the recognition of the basic semantics
> defined in the spec, namely:
> 
> Conditional zero, if condition is equal to zero
>    rd = (rs2 == 0) ? 0 : rs1
> Conditional zero, if condition is non zero
>    rd = (rs2 != 0) ? 0 : rs1
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.md: Include zicond.md
> 	* config/riscv/zicond.md: New file.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/zicond-primitiveSemantics.c: New test.
> ---
>   gcc/config/riscv/riscv.md                     |  1 +
>   gcc/config/riscv/zicond.md                    | 84 +++++++++++++++++++
>   .../riscv/zicond-primitiveSemantics.c         | 49 +++++++++++
>   3 files changed, 134 insertions(+)
>   create mode 100644 gcc/config/riscv/zicond.md
>   create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
> 
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index d63b584a4c1..6b8c2e8e268 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -3317,3 +3317,4 @@
>   (include "sifive-7.md")
>   (include "thead.md")
>   (include "vector.md")
> +(include "zicond.md")
> diff --git a/gcc/config/riscv/zicond.md b/gcc/config/riscv/zicond.md
> new file mode 100644
> index 00000000000..1cf28589c87
> --- /dev/null
> +++ b/gcc/config/riscv/zicond.md
> @@ -0,0 +1,84 @@
> +;; Machine description for the RISC-V Zicond extension
> +;; Copyright (C) 2022-23 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_code_iterator eq_or_ne [eq ne])
> +(define_code_attr eqz [(eq "nez") (ne "eqz")])
> +(define_code_attr nez [(eq "eqz") (ne "nez")])
> +
> +
> +;; Special optimization under eq/ne in primitive semantics
> +(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt1"
> +  [(set (match_operand:GPR 0 "register_operand"                   "=r")
> +        (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
> +                              (const_int 0))
> +                          (match_operand:GPR 2 "register_operand" "1")
> +                          (match_operand:GPR 3 "register_operand" "r")))]
> +  "TARGET_ZICOND && operands[1] == operands[2]"
> +  "czero.eqz\t%0,%3,%1"
Interesting.  We didn't have this pattern internally, though it's 
clever.  I'm curious how often it triggered.

Why did you need the operands[1] == operands[2] condition.   I would 
hazard a guess the idea was to reject cases that weren't going to be 
profitable if LRA/reload needed to insert copies to satisfy the matching 
constraint?

It may have been better to replace operand 2 with (match_dup 1).  If 
that isn't viable, then the right check in the condition would have been
REGNO (operands[1]) == REGNO (operands[2]).

You need to be very careful comparing REG expressions for equality like 
you did.  It probably works in this case, but it's pretty fragile in 
general.  The problem while you can compare two pseudos using pointer 
equality, you can't necessarily do that for hard registers.

What happens under the hood is you can have two distinct pseudos which 
get allocated to the same hard reg.  At assignment time we just replace 
the underlying register # without going back and fixing all the REG 
expressions.  Meaning that you can have:


(reg:XX 12) and (reg:XX 12)

Which are at distinct memory locations.  Meaning that while the RTX 
expresssions look the same, they will fail the pointer equality check.

So again, it probably works on your example, but I'd rather look for 
ways to bullet proof this better.  The (match_dup) approach is probably 
the most preferred.

Similarly for the other 3 patterns that have pointer equality tests for 
two operands in the insn condition.


Jeff
  

On 7/19/23 04:11, Xiao Zeng wrote:
> This patch completes the recognition of the basic semantics
> defined in the spec, namely:
> 
> Conditional zero, if condition is equal to zero
>    rd = (rs2 == 0) ? 0 : rs1
> Conditional zero, if condition is non zero
>    rd = (rs2 != 0) ? 0 : rs1
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.md: Include zicond.md
> 	* config/riscv/zicond.md: New file.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/zicond-primitiveSemantics.c: New test.
So I played with this a bit today.  I originally thought that using 
match_dup was the right way to go for those 4 secondary patterns.  But 
after further pondering it's not ideal.

match_dup will require pointer equality within the RTL structure.  That 
could inhibit detection in two cases.  First, SUBREGs.   SUBREGs are not 
shared.  So we'd never match if we had a SUBREG expression.

Second, post register allocation we can have the same looking RTX, but 
it may not be pointer equal.

The SUBREG issue also means that we don't want to use a REGNO (x) == 
REGNO (y) style check because those macros are only valid on REG 
expressions.  We could strip the SUBREG, but that's usually awkward to 
do in a pattern's condition.

The net result is we probably should use rtx_equal_p which I was hoping 
to avoid.  I'm testing with that change to the 4 secondary patterns 
right now.  Assuming that passes (and I have no reason to think it 
won't) then I'll go ahead and commit #1 and #2 from this series which is 
all I have time for today.



Jeff
  

On 7/19/23 04:11, Xiao Zeng wrote:
> This patch completes the recognition of the basic semantics
> defined in the spec, namely:
> 
> Conditional zero, if condition is equal to zero
>    rd = (rs2 == 0) ? 0 : rs1
> Conditional zero, if condition is non zero
>    rd = (rs2 != 0) ? 0 : rs1
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.md: Include zicond.md
> 	* config/riscv/zicond.md: New file.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/zicond-primitiveSemantics.c: New test.
So as I mentioned earlier today, adjusting the insn condition to use 
rtx_equal_p seems to be the right way to go.  Attached is a V2 of this 
patch that implements that.  It was trivial enough to do that there's no 
need to break this patch down further.

I've pushed V2 to the trunk.

Thanks,
Jeff
commit 74290c664d1d4c067a996253fe505555ec671668
Author: Xiao Zeng <zengxiao@eswincomputing.com>
Date:   Wed Jul 26 11:59:59 2023 -0600

    [PATCH 2/5] [RISC-V] Generate Zicond instruction for basic semantics
    
    This patch completes the recognition of the basic semantics
    defined in the spec, namely:
    
    Conditional zero, if condition is equal to zero
      rd = (rs2 == 0) ? 0 : rs1
    Conditional zero, if condition is non zero
      rd = (rs2 != 0) ? 0 : rs1
    
    gcc/ChangeLog:
    
            * config/riscv/riscv.md: Include zicond.md
            * config/riscv/zicond.md: New file.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.target/riscv/zicond-primitiveSemantics.c: New test.
    
            Co-authored-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
            Co-authored-by: Raphael Zinsly <rzinsly@ventanamicro.com>
            Co-authored-by: Jeff Law <jlaw@ventanamicro.com>

diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 24515bcf706..8d8fc93bb14 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -3319,3 +3319,4 @@ (define_expand "msubhisi4"
 (include "sifive-7.md")
 (include "thead.md")
 (include "vector.md")
+(include "zicond.md")
diff --git a/gcc/config/riscv/zicond.md b/gcc/config/riscv/zicond.md
new file mode 100644
index 00000000000..723a22422e1
--- /dev/null
+++ b/gcc/config/riscv/zicond.md
@@ -0,0 +1,84 @@
+;; Machine description for the RISC-V Zicond extension
+;; Copyright (C) 2022-23 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_code_iterator eq_or_ne [eq ne])
+(define_code_attr eqz [(eq "nez") (ne "eqz")])
+(define_code_attr nez [(eq "eqz") (ne "nez")])
+
+;; Zicond
+(define_insn "*czero.<eqz>.<GPR:mode><ANYI:mode>"
+  [(set (match_operand:GPR 0 "register_operand"                      "=r")
+        (if_then_else:GPR (eq_or_ne (match_operand:ANYI 1 "register_operand" "r")
+                                    (const_int 0))
+                          (match_operand:GPR 2 "register_operand"    "r")
+                          (const_int 0)))]
+  "TARGET_ZICOND"
+  "czero.<eqz>\t%0,%2,%1"
+)
+
+(define_insn "*czero.<nez>.<GPR:mode><ANYI:mode>"
+  [(set (match_operand:GPR 0 "register_operand"                     "=r")
+        (if_then_else:GPR (eq_or_ne (match_operand:ANYI 1 "register_operand" "r")
+                                    (const_int 0))
+                          (const_int 0)
+                          (match_operand:GPR 2 "register_operand"   "r")))]
+  "TARGET_ZICOND"
+  "czero.<nez>\t%0,%2,%1"
+)
+
+;; Special optimization under eq/ne in primitive semantics
+(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt1"
+  [(set (match_operand:GPR 0 "register_operand"                   "=r")
+        (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
+                              (const_int 0))
+                          (match_operand:GPR 2 "register_operand" "1")
+                          (match_operand:GPR 3 "register_operand" "r")))]
+  "TARGET_ZICOND && rtx_equal_p (operands[1], operands[2])"
+  "czero.eqz\t%0,%3,%1"
+)
+
+(define_insn "*czero.eqz.<GPR:mode><ANYI:mode>.opt2"
+  [(set (match_operand:GPR 0 "register_operand"                   "=r")
+        (if_then_else:GPR (eq (match_operand:ANYI 1 "register_operand" "r")
+                              (const_int 0))
+                          (match_operand:GPR 2 "register_operand" "r")
+                          (match_operand:GPR 3 "register_operand" "1")))]
+  "TARGET_ZICOND && rtx_equal_p (operands[1],  operands[3])"
+  "czero.nez\t%0,%2,%1"
+)
+
+(define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt3"
+  [(set (match_operand:GPR 0 "register_operand"                   "=r")
+        (if_then_else:GPR (ne (match_operand:ANYI 1 "register_operand" "r")
+                              (const_int 0))
+                          (match_operand:GPR 2 "register_operand" "r")
+                          (match_operand:GPR 3 "register_operand" "1")))]
+  "TARGET_ZICOND && rtx_equal_p (operands[1], operands[3])"
+  "czero.eqz\t%0,%2,%1"
+)
+
+(define_insn "*czero.nez.<GPR:mode><ANYI:mode>.opt4"
+  [(set (match_operand:GPR 0 "register_operand"                   "=r")
+        (if_then_else:GPR (ne (match_operand:ANYI 1 "register_operand" "r")
+                              (const_int 0))
+                          (match_operand:GPR 2 "register_operand" "1")
+                          (match_operand:GPR 3 "register_operand" "r")))]
+  "TARGET_ZICOND && rtx_equal_p (operands[1], operands[2])"
+  "czero.nez\t%0,%3,%1"
+)
diff --git a/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
new file mode 100644
index 00000000000..76c5019a992
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zicond-primitiveSemantics.c
@@ -0,0 +1,49 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mabi=ilp32f" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } {"-O0"} } */
+
+long primitiveSemantics_00(long a, long b) { return a == 0 ? 0 : b; }
+
+long primitiveSemantics_01(long a, long b) { return a != 0 ? 0 : b; }
+
+long primitiveSemantics_02(long a, long b) { return a == 0 ? b : 0; }
+
+long primitiveSemantics_03(long a, long b) { return a != 0 ? b : 0; }
+
+long primitiveSemantics_04(long a, long b) {
+  if (a)
+    b = 0;
+  return b;
+}
+
+long primitiveSemantics_05(long a, long b) {
+  if (!a)
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_06(int a, int b) { return a == 0 ? 0 : b; }
+
+int primitiveSemantics_07(int a, int b) { return a != 0 ? 0 : b; }
+
+int primitiveSemantics_08(int a, int b) { return a == 0 ? b : 0; }
+
+int primitiveSemantics_09(int a, int b) { return a != 0 ? b : 0; }
+
+int primitiveSemantics_10(int a, int b) {
+  if (a)
+    b = 0;
+  return b;
+}
+
+int primitiveSemantics_11(int a, int b) {
+  if (!a)
+    b = 0;
+  return b;
+}
+
+/* { dg-final { scan-assembler-times "czero.eqz" 6 } } */
+/* { dg-final { scan-assembler-times "czero.nez" 6 } } */
+/* { dg-final { scan-assembler-not "beq" } } */
+/* { dg-final { scan-assembler-not "bne" } } */
  

Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 7/19/23 04:11, Xiao Zeng wrote:
>> This patch completes the recognition of the basic semantics
>> defined in the spec, namely:
>> 
>> Conditional zero, if condition is equal to zero
>>    rd = (rs2 == 0) ? 0 : rs1
>> Conditional zero, if condition is non zero
>>    rd = (rs2 != 0) ? 0 : rs1
>> 
>> gcc/ChangeLog:
>> 
>> 	* config/riscv/riscv.md: Include zicond.md
>> 	* config/riscv/zicond.md: New file.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 	* gcc.target/riscv/zicond-primitiveSemantics.c: New test.
> So I played with this a bit today.  I originally thought that using 
> match_dup was the right way to go for those 4 secondary patterns.  But 
> after further pondering it's not ideal.
>
> match_dup will require pointer equality within the RTL structure.  That 
> could inhibit detection in two cases.  First, SUBREGs.   SUBREGs are not 
> shared.  So we'd never match if we had a SUBREG expression.
>
> Second, post register allocation we can have the same looking RTX, but 
> it may not be pointer equal.

Where were you seeing the requirement for pointer equality?  genrecog.cc
at least uses rtx_equal_p, and I think it has to.  E.g. some patterns
use (match_dup ...) to match output and input mems, and mem rtxes
shouldn't be shared.

I'd always understood using matching constraints against other inputs
to be a no-no, since the RA doesn't (and can't reasonably be expected to)
make two non-identical inputs match.  So AIUI, using "1" won't lead to
different code generation compared to "r".  Both are relying on the RA
happening to do the right thing.  But "1" would presumably trigger an
ICE if something goes wrong.

Thanks,
Richard

> The SUBREG issue also means that we don't want to use a REGNO (x) == 
> REGNO (y) style check because those macros are only valid on REG 
> expressions.  We could strip the SUBREG, but that's usually awkward to 
> do in a pattern's condition.
>
> The net result is we probably should use rtx_equal_p which I was hoping 
> to avoid.  I'm testing with that change to the 4 secondary patterns 
> right now.  Assuming that passes (and I have no reason to think it 
> won't) then I'll go ahead and commit #1 and #2 from this series which is 
> all I have time for today.
>
>
>
> Jeff
  

On 8/1/23 05:18, Richard Sandiford wrote:
> 
> Where were you seeing the requirement for pointer equality?  genrecog.cc
> at least uses rtx_equal_p, and I think it has to.  E.g. some patterns
> use (match_dup ...) to match output and input mems, and mem rtxes
> shouldn't be shared.
It's a general concern due to the way we handle transforming pseudos 
into hard registers after allocation is complete.   We can end up with 
two REG expressions that will compare equal according to rtx_equal_p, 
but which are not pointer equal.

For this kit I think the worst that would happen would be a failure to 
optimize cases post-reload.  But it's still good RTL hygene IMHO.



> 
> I'd always understood using matching constraints against other inputs
> to be a no-no, since the RA doesn't (and can't reasonably be expected to)
> make two non-identical inputs match.  So AIUI, using "1" won't lead to
> different code generation compared to "r".  Both are relying on the RA
> happening to do the right thing.  But "1" would presumably trigger an
> ICE if something goes wrong.
Yea, I think you're right.  The constraint isn't terribly important for 
these patterns -- the condition is really the enforcement mechanism.

Jeff
  

Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 8/1/23 05:18, Richard Sandiford wrote:
>> 
>> Where were you seeing the requirement for pointer equality?  genrecog.cc
>> at least uses rtx_equal_p, and I think it has to.  E.g. some patterns
>> use (match_dup ...) to match output and input mems, and mem rtxes
>> shouldn't be shared.
> It's a general concern due to the way we handle transforming pseudos 
> into hard registers after allocation is complete.   We can end up with 
> two REG expressions that will compare equal according to rtx_equal_p, 
> but which are not pointer equal.

But isn't that OK?  I don't think there's a requirement for match_dup
pointer equality either before or after RA.  Or at least, there
shouldn't be.  If something happens to rely on pointer equality
for match_dups then I think we should fix it.

So IMO, like you said originally, match_dup would be the right way to
handle this kind of pattern.

The reason I'm interested is that AArch64 makes pretty extensive use
of match_dup for this purpose.  E.g.:

(define_insn "aarch64_<su>abd<mode><vczle><vczbe>"
  [(set (match_operand:VDQ_BHSI 0 "register_operand" "=w")
	(minus:VDQ_BHSI
	  (USMAX:VDQ_BHSI
	    (match_operand:VDQ_BHSI 1 "register_operand" "w")
	    (match_operand:VDQ_BHSI 2 "register_operand" "w"))
	  (<max_opp>:VDQ_BHSI
	    (match_dup 1)
	    (match_dup 2))))]

So if this isn't working correctly for subregs (or for anythine else),
then I'd be keen to do something about it :)

I don't want to labour the point though.

Thanks,
Richard
  

On 8/2/23 04:05, Richard Sandiford wrote:
> Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On 8/1/23 05:18, Richard Sandiford wrote:
>>>
>>> Where were you seeing the requirement for pointer equality?  genrecog.cc
>>> at least uses rtx_equal_p, and I think it has to.  E.g. some patterns
>>> use (match_dup ...) to match output and input mems, and mem rtxes
>>> shouldn't be shared.
>> It's a general concern due to the way we handle transforming pseudos
>> into hard registers after allocation is complete.   We can end up with
>> two REG expressions that will compare equal according to rtx_equal_p,
>> but which are not pointer equal.
> 
> But isn't that OK?  I don't think there's a requirement for match_dup
> pointer equality either before or after RA.  Or at least, there
> shouldn't be.  If something happens to rely on pointer equality
> for match_dups then I think we should fix it.


> 
> So IMO, like you said originally, match_dup would be the right way to
> handle this kind of pattern.
I'd assumed that match_dup required pointer equality.  If it doesn't, 
then great, we can adjust the pattern to use match_dup.  I'm about to 
submit some bits to simplify/correct a bit of zicond.md, then I can do 
some testing with match_dup in place now that things seem to be more 
stable on the code generation correctness side.


> 
> I don't want to labour the point though.
No worries about that on my end!  I probably don't say it enough, but 
when you raise an issue, it's worth the time to make sure I understand 
your point thoroughly.

In this case I'd assumed that match_dup relied on pointer equality which 
doesn't seem to be the case.  30+ years into this codebase and I'm still 
learning new stuff!

Jeff