Re: [PATCH 2/5] [RISC-V] Generate Zicond instruction for basic semantics

[Jeff Law <jeffreyalaw@gmail.com>]

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