Re: [PATCH v3 1/1] RISC-V: Add support for 'XVentanaCondOps' reusing 'Zicond' support

[Jeff Law <jeffreyalaw@gmail.com>]

On 9/5/23 06:10, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
> 
> 'XVentanaCondOps' is a vendor extension from Ventana Micro Systems
> containing two instructions for conditional move and will be supported on
> their Veyron V1 CPU.
> 
> And most notably (for historical reasons), 'XVentanaCondOps' and the
> standard 'Zicond' extension are functionally equivalent (only encodings and
> instruction names are different).
> 
> *   czero.eqz == vt.maskc
> *   czero.nez == vt.maskcn
> 
> This commit adds support for the 'XVentanaCondOps' extension by extending
> 'Zicond' extension support.  With this, we can now reuse the optimization
> using the 'Zicond' extension for the 'XVentanaCondOps' extension.
> 
> The specification for the 'XVentanaCondOps' extension is based on:
> <https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.1/ventana-custom-extensions-v1.0.1.pdf>
> 
> gcc/ChangeLog:
> 
> 	* common/config/riscv/riscv-common.cc (riscv_ext_flag_table):
> 	Parse 'XVentanaCondOps' extension.
> 	* config/riscv/riscv-opts.h (MASK_XVENTANACONDOPS): New.
> 	(TARGET_XVENTANACONDOPS): Ditto.
> 	(TARGET_ZICOND_LIKE): New to represent targets with conditional
> 	moves like 'Zicond'.  It includes RV64 + 'XVentanaCondOps'.
> 	* config/riscv/riscv.cc (riscv_rtx_costs): Replace TARGET_ZICOND
> 	with TARGET_ZICOND_LIKE.
> 	(riscv_expand_conditional_move): Ditto.
> 	* config/riscv/riscv.md (mov<mode>cc): Replace TARGET_ZICOND with
> 	TARGET_ZICOND_LIKE.
> 	* config/riscv/riscv.opt: Add new riscv_xventana_subext.
> 	* config/riscv/zicond.md: Modify description.
> 	(eqz_ventana): New to match corresponding czero instructions.
> 	(nez_ventana): Ditto.
> 	(*czero.<eqz>.<GPR><X>): Emit a 'XVentanaCondOps' instruction if
> 	'Zicond' is not available but 'XVentanaCondOps' + RV64 is.
> 	(*czero.<eqz>.<GPR><X>): Ditto.
> 	(*czero.eqz.<GPR><X>.opt1): Ditto.
> 	(*czero.nez.<GPR><X>.opt2): Ditto.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/xventanacondops-primitiveSemantics.c: New test,
> 	modified from zicond-primitiveSemantics.c.
> 	* gcc.target/riscv/xventanacondops-primitiveSemantics-rv32.c: New
> 	test to make sure that XVentanaCondOps instructions are disabled
> 	on RV32.
> 	* gcc.target/riscv/xventanacondops-xor-01.c: New test, modified
> 	from zicond-xor-01.c.
> ---
>   gcc/common/config/riscv/riscv-common.cc       |  2 +
       \
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 8d8f7b4f16ed..eb10f4a3323f 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -2745,7 +2745,7 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
>   	  *total = COSTS_N_INSNS (1);
>   	  return true;
>   	}
> -      else if (TARGET_ZICOND
> +      else if (TARGET_ZICOND_LIKE
Internally we have this as:

(TARGET_ZICOND || TARGET_XVENTANACONDOPS)

I don't really care, so I'm happy to go with yours.


> +(define_code_attr eqz_ventana [(eq "maskcn") (ne "maskc")])
> +(define_code_attr nez_ventana [(eq "maskc") (ne "maskcn")])
We did these as N/n which output n or nothing:

(define_code_attr n [(eq "n") (ne "")])
(define_code_attr N [(eq "") (ne "n")])


>   
>   ;; Zicond
>   (define_insn "*czero.<eqz>.<GPR:mode><X:mode>"
> @@ -28,8 +31,15 @@
>                                       (const_int 0))
>                             (match_operand:GPR 2 "register_operand"    "r")
>                             (const_int 0)))]
> -  "TARGET_ZICOND"
> -  "czero.<eqz>\t%0,%2,%1"
> +  "TARGET_ZICOND_LIKE"
> +  {
> +    if (TARGET_ZICOND)
> +      return "czero.<eqz>\t%0,%2,%1";
> +    else if (TARGET_XVENTANACONDOPS && TARGET_64BIT)
> +      return "vt.<eqz_ventana>\t%0,%2,%1";
> +    else
> +      gcc_unreachable ();
> +  }
>   )
And so the output template ends up like this:

>   "* return TARGET_ZICOND ? \"czero.<eqz>\t%0,%2,%1\" : \"vt.maskc<n>\t%0,%2,%1\"; "

But again, I don't care enough about this to make it a big deal and I'm 
happy to go with your approach.



> diff --git a/gcc/testsuite/gcc.target/riscv/xventanacondops-primitiveSemantics-rv32.c b/gcc/testsuite/gcc.target/riscv/xventanacondops-primitiveSemantics-rv32.c
> new file mode 100644
> index 000000000000..992f1425c54f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/xventanacondops-primitiveSemantics-rv32.c
So we're never going to have an rv32 variant.  So I don't think we need 
rv32 xventanacondops tests.

For the tests we keep, the right way to do them is with #includes.

ie start with this:



> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gc_xventanacondops -mabi=lp64d" } */
> +/* { dg-skip-if "" { *-*-* } {"-O0" "-Og"} } */

Then #include the zicond variant of the test


> +
> +/* { dg-final { scan-assembler-times "vt\\.maskc\t" 6 } } */
> +/* { dg-final { scan-assembler-times "vt\\.maskcn\t" 6 } } */
> +/* { dg-final { scan-assembler-not "beq" } } */
> +/* { dg-final { scan-assembler-not "bne" } } */
Then you have the assembly scan strings.

That way we don't duplicate the actual test code.

If you could fixup the tests, then I think this will be ready to integrate.

Thanks,
jeff