From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender-0.a4lg.com [IPv6:2401:2500:203:30b:4000:6bfe:4757:0]) by sourceware.org (Postfix) with ESMTPS id 906723858D28 for ; Wed, 6 Sep 2023 02:33:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 906723858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=irq.a4lg.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=irq.a4lg.com Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id 1E5CB300089; Wed, 6 Sep 2023 02:33:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1693967621; bh=FfF8LvtSg+D2iM7gmReC83bH//QySGz2EI0ug/dB5fk=; h=Message-ID:Date:Mime-Version:Subject:To:References:Cc:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=it4PPeDmaxLakPT9ACupADBOCyajQ94HVuTI99uTS820eolzSMEZcMHG2wL97gmhB UYmQ1GLB3lATgrH4FFktWtc90UIQVqbz+JaJu1UhHo3nqm4fjWXwbbia+OA+R/lEce 9PGMp7HYWrnnC3g63kgrldI/4FX2IH45Dx/iQzlw= Message-ID: <32677516-2795-4e8f-a8d0-8304ba641c3a@irq.a4lg.com> Date: Wed, 6 Sep 2023 11:33:40 +0900 Mime-Version: 1.0 Subject: Re: [PATCH v3 1/1] RISC-V: Add support for 'XVentanaCondOps' reusing 'Zicond' support Content-Language: en-US To: Jeff Law References: <3cd84da2-fe41-4a89-850c-84f3b7b26bca@gmail.com> Cc: GCC Patches From: Tsukasa OI In-Reply-To: <3cd84da2-fe41-4a89-850c-84f3b7b26bca@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023/09/06 9:17, Jeff Law wrote: > > > On 9/5/23 06:10, Tsukasa OI wrote: >> From: Tsukasa OI >> >> '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: >> >> >> 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 (movcc): 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..): Emit a 'XVentanaCondOps' instruction if >>     'Zicond' is not available but 'XVentanaCondOps' + RV64 is. >>     (*czero..): Ditto. >>     (*czero.eqz..opt1): Ditto. >>     (*czero.nez..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.." >> @@ -28,8 +31,15 @@ >>                                       (const_int 0)) >>                             (match_operand:GPR 2 "register_operand"    >> "r") >>                             (const_int 0)))] >> -  "TARGET_ZICOND" >> -  "czero.\t%0,%2,%1" >> +  "TARGET_ZICOND_LIKE" >> +  { >> +    if (TARGET_ZICOND) >> +      return "czero.\t%0,%2,%1"; >> +    else if (TARGET_XVENTANACONDOPS && TARGET_64BIT) >> +      return "vt.\t%0,%2,%1"; >> +    else >> +      gcc_unreachable (); >> +  } >>   ) > And so the output template ends up like this: > >>   "* return TARGET_ZICOND ? \"czero.\t%0,%2,%1\" : >> \"vt.maskc\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