> On Oct 15, 2024, at 20:11, Andrew Carlotti <andrew.carlo...@arm.com> wrote:
>
> On Tue, Oct 15, 2024 at 02:18:43PM +0800, Yangyu Chen wrote:
>> Some architectures may use ',' in the attribute string, but it is not
>> used as the separator for different targets. To avoid conflict, we
>> introduce a new macro TARGET_FMV_ATTR_SEPARATOR to separate different
>> clones.
>
> This is only for the target_clones attribute, so how about calling it
> TARGET_CLONES_ATTR_SEPARATOR instead (or pluralised - see below)?
>
Sound like a good idea. I choose to use TARGET_CLONES_ATTR_SEPARATOR in the next
revision.
Link:
https://patchwork.sourceware.org/project/gcc/patch/20241015181607.3689413-1-chenyan...@isrc.iscas.ac.cn/
>> As an example, according to RISC-V C-API Specification [1], RISC-V allows
>> ',' in the attribute string in the "arch=" option to specify one more
>> ISA extensions in the same target function, which conflict with the
>> default separator to separate different clones. This patch introduces
>> TARGET_FMV_ATTR_SEPARATOR for RISC-V and choose '#' as the separator,
>> since '#' is not allowed in the target_clones option string.
>>
>> [1]
>> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/c6c5d6d9cf96b342293315a5dff3d25e96ef8191/src/c-api.adoc#__attribute__targetattr-string
>>
>> gcc/ChangeLog:
>>
>> * defaults.h (TARGET_FMV_ATTR_SEPARATOR): Define new macro.
>> * multiple_target.cc (get_attr_str): Use
>> TARGET_FMV_ATTR_SEPARATOR to separate attributes.
>> (separate_attrs): Likewise.
>> * config/riscv/riscv.h (TARGET_FMV_ATTR_SEPARATOR): Define
>> TARGET_FMV_ATTR_SEPARATOR for RISC-V.
>> ---
>> gcc/config/riscv/riscv.h | 5 +++++
>> gcc/defaults.h | 4 ++++
>> gcc/multiple_target.cc | 19 ++++++++++++-------
>> 3 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
>> index ca1b8329cdc..858cab72a4c 100644
>> --- a/gcc/config/riscv/riscv.h
>> +++ b/gcc/config/riscv/riscv.h
>> @@ -1298,4 +1298,9 @@ extern void riscv_remove_unneeded_save_restore_calls
>> (void);
>> STACK_BOUNDARY / BITS_PER_UNIT) \
>> : (crtl->outgoing_args_size + STACK_POINTER_OFFSET))
>>
>> +/* According to the RISC-V C API, the arch string may contains ','. To avoid
>> + the conflict with the default separator, we choose '#' as the separator
>> for
>> + the target attribute. */
>> +#define TARGET_FMV_ATTR_SEPARATOR '#'
>> +
>> #endif /* ! GCC_RISCV_H */
>> diff --git a/gcc/defaults.h b/gcc/defaults.h
>> index ac2d25852ab..f451efcb33e 100644
>> --- a/gcc/defaults.h
>> +++ b/gcc/defaults.h
>> @@ -874,6 +874,10 @@ see the files COPYING3 and COPYING.RUNTIME
>> respectively. If not, see
>> #define TARGET_HAS_FMV_TARGET_ATTRIBUTE 1
>> #endif
>>
>> +/* Select a attribute separator for function multiversioning. */
>> +#ifndef TARGET_FMV_ATTR_SEPARATOR
>> +#define TARGET_FMV_ATTR_SEPARATOR ','
>> +#endif
>>
>> /* Select a format to encode pointers in exception handling data. We
>> prefer those that result in fewer dynamic relocations. Assume no
>> diff --git a/gcc/multiple_target.cc b/gcc/multiple_target.cc
>> index 1fdd279da04..5a056b44571 100644
>> --- a/gcc/multiple_target.cc
>> +++ b/gcc/multiple_target.cc
>> @@ -180,7 +180,7 @@ create_dispatcher_calls (struct cgraph_node *node)
>> }
>> }
>>
>> -/* Create string with attributes separated by comma.
>> +/* Create string with attributes separated by TARGET_FMV_ATTR_SEPARATOR.
>> Return number of attributes. */
>>
>> static int
>> @@ -194,17 +194,21 @@ get_attr_str (tree arglist, char *attr_str)
>> {
>> const char *str = TREE_STRING_POINTER (TREE_VALUE (arg));
>> size_t len = strlen (str);
>> - for (const char *p = strchr (str, ','); p; p = strchr (p + 1, ','))
>> + for (const char *p = strchr (str, TARGET_FMV_ATTR_SEPARATOR);
>> + p;
>> + p = strchr (p + 1, TARGET_FMV_ATTR_SEPARATOR))
>> argnum++;
>> memcpy (attr_str + str_len_sum, str, len);
>> - attr_str[str_len_sum + len] = TREE_CHAIN (arg) ? ',' : '\0';
>> + attr_str[str_len_sum + len]
>> + = TREE_CHAIN (arg) ? TARGET_FMV_ATTR_SEPARATOR : '\0';
>> str_len_sum += len + 1;
>> argnum++;
>> }
>> return argnum;
>> }
>>
>> -/* Return number of attributes separated by comma and put them into ARGS.
>> +/* Return number of attributes separated by TARGET_FMV_ATTR_SEPARATOR and
>> put
>> + them into ARGS.
>> If there is no DEFAULT attribute return -1.
>> If there is an empty string in attribute return -2.
>> If there are multiple DEFAULT attributes return -3.
>> @@ -215,9 +219,10 @@ separate_attrs (char *attr_str, char **attrs, int
>> attrnum)
>> {
>> int i = 0;
>> int default_count = 0;
>> + char separator_str[] = {TARGET_FMV_ATTR_SEPARATOR, '\0'};
>
> How about defining the macro as a string (and appending an S to the name -
> e.g.
> TARGET_CLONES_ATTR_SEPARATORS)?
>
I didn't find a clean way in C to build a char array based on a single char. I
searched some codes in GCC, and I found a similar use case for DIR_SEPARATOR
which is:
static const char xxxx_str[] = { xxxx, 0 };
So, I edit this to:
static const char separator_str[] = { TARGET_CLONES_ATTR_SEPARATOR, 0 };
I think this way is better.
Thanks,
Yangyu Chen
>> - for (char *attr = strtok (attr_str, ",");
>> - attr != NULL; attr = strtok (NULL, ","))
>> + for (char *attr = strtok (attr_str, separator_str);
>> + attr != NULL; attr = strtok (NULL, separator_str))
>> {
>> if (strcmp (attr, "default") == 0)
>> {
>> @@ -305,7 +310,7 @@ static bool
>> expand_target_clones (struct cgraph_node *node, bool definition)
>> {
>> int i;
>> - /* Parsing target attributes separated by comma. */
>> + /* Parsing target attributes separated by TARGET_FMV_ATTR_SEPARATOR. */
>> tree attr_target = lookup_attribute ("target_clones",
>> DECL_ATTRIBUTES (node->decl));
>> /* No targets specified. */
>> --
>> 2.45.2
