Hi, Thanks for the update. It looks like there are some unaddressed comments from the v8 review:
chenglulu <chengl...@loongson.cn> 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/larchintrin.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 > +<http://www.gnu.org/licenses/>. */ > + > +#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]"=&r"(drdtime.dvalue),[tid]"=&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 “public”? In other words, can users 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]"=&r"(__drdtime.dvalue),[tid]"=&r"(__drdtime.dtimeid) :); return __drdtime; } Same idea for… > + > +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]"=&r"(rdtime.value),[tid]"=&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]"=&r"(rdtime.value),[tid]"=&r"(rdtime.timeid) > + :); > + return rdtime; > +} > +#define __rdtimel_w __builtin_loongarch_rdtimel_w …these two functions, and for the later functions where you have a __builtin_* function defined directly in the header file. > […] > +/* 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_<X>. */ > +#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_<X>. */ > +#define STRINGIFY(X) #X > +const char *const > +loongarch_fp_conditions[16]= {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 “cond” 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. > […] > +/* 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 “loongarch_” 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_IBAR)] "" "ibar\t%0") to: (define_insn "loongarch_ibar" [(unspec_volatile:SI [(match_operand 0 "const_uimm15_operand")] UNSPECV_IBAR)] "" "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 sqrt<mode>2 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 “private” instruction names like 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
