Re: [PATCH v8 06/12] LoongArch Port: Builtin functions.

[Richard Sandiford <richard.sandiford@arm.com>]

xuchenghua@loongson.cn writes:
> +#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

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.

> […]
> +#if defined __loongarch64
> +/* Assembly instruction format:	ui5, rj, si12.  */
> +/* Data types in instruction templates:  VOID, USI, UDI, SI.  */
> +#define __dcacop(/*ui5*/ _1, /*unsigned long int*/ _2, /*si12*/ _3) \
> +  ((void) __builtin_loongarch_dcacop ((_1), (unsigned long int) (_2), (_3)))
> +#else
> +#error "Don't support this ABI."

“Unsupported ABI” might be better.  Same for the rest of the file.

> +#endif
> […]
> +/* 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.

> +
> +/* Declare an availability predicate for built-in functions that require
> + * COND to be true.  NAME is the main part of the predicate's name.  */

Formatting nit: GNU style is not to have the “*” at the start
of the line.

> +#define AVAIL_ALL(NAME, COND) \
> +  static unsigned int \
> +  loongarch_builtin_avail_##NAME (void) \
> +  { \
> +    return (COND) ? 1 : 0; \
> +  }
> +
> +static unsigned int
> +loongarch_builtin_avail_default (void)
> +{
> +  return 1;
> +}
> +/* This structure describes a single built-in function.  */
> +struct loongarch_builtin_description

Very minor nit, sorry, but: missing blank line before the comment.

> […]
> +/* 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.

Looks good to me otherwise, thanks.

Richard