From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id A2819385842E for ; Tue, 22 Mar 2022 11:24:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A2819385842E Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3795F153B; Tue, 22 Mar 2022 04:24:36 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 82F4A3F66F; Tue, 22 Mar 2022 04:24:35 -0700 (PDT) From: Richard Sandiford To: chenglulu Mail-Followup-To: chenglulu , gcc-patches@gcc.gnu.org, xuchenghua@loongson.cn, joseph@codesourcery.com, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, xuchenghua@loongson.cn, joseph@codesourcery.com Subject: Re: [PATCH v9 06/12] LoongArch Port: Builtin functions. References: <20220319092425.3324697-1-chenglulu@loongson.cn> <20220319092425.3324697-7-chenglulu@loongson.cn> Date: Tue, 22 Mar 2022 11:24:34 +0000 In-Reply-To: <20220319092425.3324697-7-chenglulu@loongson.cn> (chenglulu@loongson.cn's message of "Sat, 19 Mar 2022 17:24:19 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Mar 2022 11:24:38 -0000 Hi, Thanks for the update. It looks like there are some unaddressed comments from the v8 review: chenglulu writes: > gcc/ > > * config/loongarch/larchintrin.h: New file. > * config/loongarch/loongarch-builtins.cc: New file. > --- > gcc/config/loongarch/larchintrin.h | 409 +++++++++++++++++ > gcc/config/loongarch/loongarch-builtins.cc | 511 +++++++++++++++++++++ > 2 files changed, 920 insertions(+) > create mode 100644 gcc/config/loongarch/larchintrin.h > create mode 100644 gcc/config/loongarch/loongarch-builtins.cc > > diff --git a/gcc/config/loongarch/larchintrin.h b/gcc/config/loongarch/la= rchintrin.h > new file mode 100644 > index 00000000000..fa63c6606bc > --- /dev/null > +++ b/gcc/config/loongarch/larchintrin.h > @@ -0,0 +1,409 @@ > +/* Intrinsics for LoongArch BASE operations. > + Copyright (C) 2021-2022 Free Software Foundation, Inc. > + Contributed by Loongson Ltd. > + > +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. > + > +Under Section 7 of GPL version 3, you are granted additional > +permissions described in the GCC Runtime Library Exception, version > +3.1, as published by the Free Software Foundation. > + > +You should have received a copy of the GNU General Public License and > +a copy of the GCC Runtime Library Exception along with this program; > +see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > +. */ > + > +#ifndef _GCC_LOONGARCH_BASE_INTRIN_H > +#define _GCC_LOONGARCH_BASE_INTRIN_H > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +typedef struct drdtime > +{ > + unsigned long dvalue; > + unsigned long dtimeid; > +} __drdtime_t; > + > +typedef struct rdtime > +{ > + unsigned int value; > + unsigned int timeid; > +} __rdtime_t; > + > +#ifdef __loongarch64 > +extern __inline __drdtime_t > +__attribute__ ((__gnu_inline__, __always_inline__, __artificial__)) > +__builtin_loongarch_rdtime_d (void) > +{ > + __drdtime_t drdtime; > + __asm__ volatile ( > + "rdtime.d\t%[val],%[tid]\n\t" > + : [val]"=3D&r"(drdtime.dvalue),[tid]"=3D&r"(drdtime.dtimeid) > + :); > + return drdtime; It's usually better to use __foo names for local variables and parameters, in case the user defines a macro called (in this case) drdtime. > +} > +#define __rdtime_d __builtin_loongarch_rdtime_d > +#endif Are both of these names =E2=80=9Cpublic=E2=80=9D? In other words, can user= s use __builtin_longarch_rdtime_d directly, instead of using __rdtime_d? If only __rdtime_d is public then it might be better to define the function directly, since that will give better error messages. Putting the previous two comments together, I was thinking of something like this: __attribute__ ((__gnu_inline__, __always_inline__, __artificial__)) __rdtime_d (void) { __drdtime_t __drdtime; __asm__ volatile ( "rdtime.d\t%[val],%[tid]\n\t" : [val]"=3D&r"(__drdtime.dvalue),[tid]"=3D&r"(__drdtime.dtimeid) :); return __drdtime; } Same idea for=E2=80=A6 > + > +extern __inline __rdtime_t > +__attribute__ ((__gnu_inline__, __always_inline__, __artificial__)) > +__builtin_loongarch_rdtimeh_w (void) > +{ > + __rdtime_t rdtime; > + __asm__ volatile ( > + "rdtimeh.w\t%[val],%[tid]\n\t" > + : [val]"=3D&r"(rdtime.value),[tid]"=3D&r"(rdtime.timeid) > + :); > + return rdtime; > +} > +#define __rdtimeh_w __builtin_loongarch_rdtimeh_w > + > +extern __inline __rdtime_t > +__attribute__ ((__gnu_inline__, __always_inline__, __artificial__)) > +__builtin_loongarch_rdtimel_w (void) > +{ > + __rdtime_t rdtime; > + __asm__ volatile ( > + "rdtimel.w\t%[val],%[tid]\n\t" > + : [val]"=3D&r"(rdtime.value),[tid]"=3D&r"(rdtime.timeid) > + :); > + return rdtime; > +} > +#define __rdtimel_w __builtin_loongarch_rdtimel_w =E2=80=A6these two functions, and for the later functions where you have a __builtin_* function defined directly in the header file. > [=E2=80=A6] > +/* Invoke MACRO (COND) for each fcmp.cond.{s/d} condition. */ > +#define LARCH_FP_CONDITIONS(MACRO) \ > + MACRO (f), \ > + MACRO (un), \ > + MACRO (eq), \ > + MACRO (ueq), \ > + MACRO (olt), \ > + MACRO (ult), \ > + MACRO (ole), \ > + MACRO (ule), \ > + MACRO (sf), \ > + MACRO (ngle), \ > + MACRO (seq), \ > + MACRO (ngl), \ > + MACRO (lt), \ > + MACRO (nge), \ > + MACRO (le), \ > + MACRO (ngt) > + > +/* Enumerates the codes above as LARCH_FP_COND_. */ > +#define DECLARE_LARCH_COND(X) LARCH_FP_COND_##X > +enum loongarch_fp_condition > +{ > + LARCH_FP_CONDITIONS (DECLARE_LARCH_COND) > +}; > +#undef DECLARE_LARCH_COND > + > +/* Index X provides the string representation of LARCH_FP_COND_. */ > +#define STRINGIFY(X) #X > +const char *const > +loongarch_fp_conditions[16]=3D {LARCH_FP_CONDITIONS (STRINGIFY)}; > +#undef STRINGIFY It doesn't look like the code above is needed, since none of the current built-ins have a condition code attached. Same applies to the later =E2=80=9Ccond=E2=80=9D field and related comments. What I meant is that, for MIPS, this code was needed by: /* Define all the built-in functions related to C.cond.fmt condition COND. = */ #define CMP_BUILTINS(COND) \ MOVTF_BUILTINS (c, COND, paired_single), \ MOVTF_BUILTINS (cabs, COND, mips3d), \ CMP_SCALAR_BUILTINS (cabs, COND, mips3d), \ CMP_PS_BUILTINS (c, COND, paired_single), \ CMP_PS_BUILTINS (cabs, COND, mips3d), \ CMP_4S_BUILTINS (c, COND), \ CMP_4S_BUILTINS (cabs, COND) This produced different c and cabs functions for every FP condition code. But LoongArch doesn't seem to have any built-in functions like this, so I think it would be better to drop the code. > [=E2=80=A6] > +/* Loongson support crc. */ > +#define CODE_FOR_loongarch_crc_w_b_w CODE_FOR_crc_w_b_w > +#define CODE_FOR_loongarch_crc_w_h_w CODE_FOR_crc_w_h_w > +#define CODE_FOR_loongarch_crc_w_w_w CODE_FOR_crc_w_w_w > +#define CODE_FOR_loongarch_crc_w_d_w CODE_FOR_crc_w_d_w > +#define CODE_FOR_loongarch_crcc_w_b_w CODE_FOR_crcc_w_b_w > +#define CODE_FOR_loongarch_crcc_w_h_w CODE_FOR_crcc_w_h_w > +#define CODE_FOR_loongarch_crcc_w_w_w CODE_FOR_crcc_w_w_w > +#define CODE_FOR_loongarch_crcc_w_d_w CODE_FOR_crcc_w_d_w > + > +/* Privileged state instruction. */ > +#define CODE_FOR_loongarch_cpucfg CODE_FOR_cpucfg > +#define CODE_FOR_loongarch_asrtle_d CODE_FOR_asrtle_d > +#define CODE_FOR_loongarch_asrtgt_d CODE_FOR_asrtgt_d > +#define CODE_FOR_loongarch_csrrd CODE_FOR_csrrd > +#define CODE_FOR_loongarch_dcsrrd CODE_FOR_dcsrrd > +#define CODE_FOR_loongarch_csrwr CODE_FOR_csrwr > +#define CODE_FOR_loongarch_dcsrwr CODE_FOR_dcsrwr > +#define CODE_FOR_loongarch_csrxchg CODE_FOR_csrxchg > +#define CODE_FOR_loongarch_dcsrxchg CODE_FOR_dcsrxchg > +#define CODE_FOR_loongarch_iocsrrd_b CODE_FOR_iocsrrd_b > +#define CODE_FOR_loongarch_iocsrrd_h CODE_FOR_iocsrrd_h > +#define CODE_FOR_loongarch_iocsrrd_w CODE_FOR_iocsrrd_w > +#define CODE_FOR_loongarch_iocsrrd_d CODE_FOR_iocsrrd_d > +#define CODE_FOR_loongarch_iocsrwr_b CODE_FOR_iocsrwr_b > +#define CODE_FOR_loongarch_iocsrwr_h CODE_FOR_iocsrwr_h > +#define CODE_FOR_loongarch_iocsrwr_w CODE_FOR_iocsrwr_w > +#define CODE_FOR_loongarch_iocsrwr_d CODE_FOR_iocsrwr_d > +#define CODE_FOR_loongarch_lddir CODE_FOR_lddir > +#define CODE_FOR_loongarch_dlddir CODE_FOR_dlddir > +#define CODE_FOR_loongarch_ldpte CODE_FOR_ldpte > +#define CODE_FOR_loongarch_dldpte CODE_FOR_dldpte > +#define CODE_FOR_loongarch_cacop CODE_FOR_cacop > +#define CODE_FOR_loongarch_dcacop CODE_FOR_dcacop > +#define CODE_FOR_loongarch_dbar CODE_FOR_dbar > +#define CODE_FOR_loongarch_ibar CODE_FOR_ibar Unless there's a reason not to, it would be better to add =E2=80=9Cloongarc= h_=E2=80=9D to the names of the .md patterns instead. That removes the need for the list above, but it also reduces the risk of an accidental clash with target-independent pattern names. For example, you could change: (define_insn "ibar" [(unspec_volatile:SI [(match_operand 0 "const_uimm15_operand")] UNSPECV_I= BAR)] "" "ibar\t%0") to: (define_insn "loongarch_ibar" [(unspec_volatile:SI [(match_operand 0 "const_uimm15_operand")] UNSPECV_I= BAR)] "" "ibar\t%0") and remove the #define above. Same idea for the others. The MIPS list contains things like: #define CODE_FOR_mips_sqrt_ps CODE_FOR_sqrtv2sf2 #define CODE_FOR_mips_addq_ph CODE_FOR_addv2hi3 #define CODE_FOR_mips_addu_qb CODE_FOR_addv4qi3 #define CODE_FOR_mips_subq_ph CODE_FOR_subv2hi3 #define CODE_FOR_mips_subu_qb CODE_FOR_subv4qi3 The reason for this is that sqrt2 is an optab that target-independent code understands. The #define is therefore mapping the built-in function to the optab. But this isn't necessary for =E2=80=9Cprivate=E2=80=9D instruction names li= ke ibar. It can actually be counter-productive to use names like ibar for private instructions, since if a target-independent ibar pattern is added in future, it might not have the same semantics as the LoongArch instruction. Thanks, Richard