On 2023/09/06 9:17, Jeff Law wrote:
>
>
> On 9/5/23 06:10, Tsukasa OI wrote:
>> From: Tsukasa OI <research_tra...@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
