Re: [patch] 19/n: trans-mem: compiler tree/gimple stuff

[Richard Guenther <richard dot guenther at gmail dot com>]

On Sun, Nov 6, 2011 at 12:16 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> [rth, see below]
>
>>> Index: gcc/attribs.c
>>> ===================================================================
>>> --- gcc/attribs.c ? ? ? (.../trunk) ? ? (revision 180744)
>>> +++ gcc/attribs.c ? ? ? (.../branches/transactional-memory) ? ? (revision
>>> 180773)
>>> @@ -166,7 +166,8 @@ init_attributes (void)
>>> ? ? ? ? ?gcc_assert (strcmp (attribute_tables[i][j].name,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?attribute_tables[i][k].name));
>>> ? ? }
>>> - ?/* Check that no name occurs in more than one table. ?*/
>>> + ?/* Check that no name occurs in more than one table. ?Names that
>>> + ? ? begin with '*' are exempt, and may be overridden. ?*/
>>> ? for (i = 0; i< ?ARRAY_SIZE (attribute_tables); i++)
>>> ? ? {
>>> ? ? ? size_t j, k, l;
>>> @@ -174,8 +175,9 @@ init_attributes (void)
>>> ? ? ? for (j = i + 1; j< ?ARRAY_SIZE (attribute_tables); j++)
>>> ? ? ? ?for (k = 0; attribute_tables[i][k].name != NULL; k++)
>>> ? ? ? ? ?for (l = 0; attribute_tables[j][l].name != NULL; l++)
>>> - ? ? ? ? ? gcc_assert (strcmp (attribute_tables[i][k].name,
>>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? attribute_tables[j][l].name));
>>> + ? ? ? ? ? gcc_assert (attribute_tables[i][k].name[0] == '*'
>>> + ? ? ? ? ? ? ? ? ? ? ? || strcmp (attribute_tables[i][k].name,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?attribute_tables[j][l].name));
>>> ? ? }
>>> ?#endif
>>>
>>> @@ -207,7 +209,7 @@ register_attribute (const struct attribu
>>> ? slot = htab_find_slot_with_hash (attribute_hash,&str,
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? substring_hash (str.str, str.length),
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? INSERT);
>>> - ?gcc_assert (!*slot);
>>> + ?gcc_assert (!*slot || attr->name[0] == '*');
>>> ? *slot = (void *) CONST_CAST (struct attribute_spec *, attr);
>>> ?}
>>
>> The above changes seem to belong to a different changeset and look
>> strange. ?Why would attributes ever appear in two different tables?
>
> I couldn't find a corresponding gcc-patches message for this patch, but I
> was able to hunt down full the patch, which I am attaching for discussion.
>
> This seems to be a change required for allowing '*' to override builtins, so
> it is indeed part of the branch. ?Perhaps with the full context it is easier
> to review.

Ah, indeed ...

> I will defer to rth to answer any questions on the original motivation.
>
> Richi, do you have any particular issue with the attribs.c change? ?Does
> this context resolve any questions you may have had?

... no, it just looked weird without seeing a use.  Now, target specific
attributes on a non-target specific builtin are of course weird.  Which
explains the patch, sort-of.  Still feels like a hack, but I can't think
of anything better, other than a target hook that we'd call for
all middle-end builtins we generate and which would allow target specific
modifications.  No idea if that would be better.  I'll defer to rth for this.

Richard.

> Aldy
>