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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

On 2023/09/06 9:17, Jeff Law wrote:
> 
> 
> 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.

Because XVentanaCondOps instructions are only available on 64-bit target
(I wanted to prevent misuses because we don't reject XVentanaCondOps on
RV32), the target expression would be:

(a) (TARGET_ZICOND || (TARGET_XVENTANACONDOPS && TARGET_64BIT))

and I had three options to deal with it.

1.  Use the plain expression (a)
2.  Name the expression (a)
3.  Enable TARGET_XVENTANACONDOPS only on 64-bit target

I think option 2 is the simplest yet understandable.

> 
> 
>> +(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")])

That's a great idea.  I will stick to "eqz_ventana" and "nez_ventana"
but will change the contents to "n" and "".

> 
> 
>>     ;; 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
> 

I'm happy to hear that because I had no confidence so whether we can use
#include to share common parts.  I haven't tried yet but I believe we
have to #include only common parts (not including dg instructions
containing -march=..._zicond) so I will likely required to modify zicond
tests as well.

I'll submit PATCH v4 (not committing directly) as changes will be a bit
larger (and Jeff's words seem "near approval" even after fixing the
tests, not complete approval).

Thanks,
Tsukasa