On Mon, Jul 17, 2023 at 10:21 AM Richard Sandiford <richard.sandif...@arm.com> wrote: > > Richard Biener <richard.guent...@gmail.com> writes: > > On Fri, Jul 14, 2023 at 5:58 PM Richard Sandiford via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> > >> Summary: We'd like to be able to specify some attributes using > >> keywords, rather than the traditional __attribute__ or [[...]] > >> syntax. Would that be OK? > >> > >> In more detail: > >> > >> We'd like to add some new target-specific attributes for Arm SME. > >> These attributes affect semantics and code generation and so they > >> can't simply be ignored. > >> > >> Traditionally we've done this kind of thing by adding GNU attributes, > >> via TARGET_ATTRIBUTE_TABLE in GCC's case. The problem is that both > >> GCC and Clang have traditionally only warned about unrecognised GNU > >> attributes, rather than raising an error. Older compilers might > >> therefore be able to look past some uses of the new attributes and > >> still produce object code, even though that object code is almost > >> certainly going to be wrong. (The compilers will also emit a default-on > >> warning, but that might go unnoticed when building a big project.) > >> > >> There are some existing attributes that similarly affect semantics > >> in ways that cannot be ignored. vector_size is one obvious example. > >> But that doesn't make it a good thing. :) > >> > >> Also, C++ says this for standard [[...]] attributes: > >> > >> For an attribute-token (including an attribute-scoped-token) > >> not specified in this document, the behavior is implementation-defined; > >> any such attribute-token that is not recognized by the implementation > >> is ignored. > >> > >> which doubles down on the idea that attributes should not be used > >> for necessary semantic information. > >> > >> One of the attributes we'd like to add provides a new way of compiling > >> existing code. The attribute doesn't require SME to be available; > >> it just says that the code must be compiled so that it can run in either > >> of two modes. This is probably the most dangerous attribute of the set, > >> since compilers that ignore it would just produce normal code. That > >> code might work in some test scenarios, but it would fail in others. > >> > >> The feeling from the Clang community was therefore that these SME > >> attributes should use keywords instead, so that the keywords trigger > >> an error with older compilers. > >> > >> However, it seemed wrong to define new SME-specific grammar rules, > >> since the underlying problem is pretty generic. We therefore > >> proposed having a type of keyword that can appear exactly where > >> a standard [[...]] attribute can appear and that appertains to > >> exactly what a standard [[...]] attribute would appertain to. > >> No divergence or cherry-picking is allowed. > >> > >> For example: > >> > >> [[arm::foo]] > >> > >> would become: > >> > >> __arm_foo > >> > >> and: > >> > >> [[arm::bar(args)]] > >> > >> would become: > >> > >> __arm_bar(args) > >> > >> It wouldn't be possible to retrofit arguments to a keyword that > >> previously didn't take arguments, since that could lead to parsing > >> ambiguities. So when a keyword is first added, a binding decision > >> would need to be made whether the keyword always takes arguments > >> or is always standalone. > >> > >> For that reason, empty argument lists are allowed for keywords, > >> even though they're not allowed for [[...]] attributes. > >> > >> The argument-less version was accepted into Clang, and I have a follow-on > >> patch for handling arguments. Would the same thing be OK for GCC, > >> in both the C and C++ frontends? > >> > >> The patch below is a proof of concept for the C frontend. It doesn't > >> bootstrap due to warnings about uninitialised fields. And it doesn't > >> have tests. But I did test it locally with various combinations of > >> attribute_spec and it seemed to work as expected. > >> > >> The impact on the C frontend seems to be pretty small. It looks like > >> the impact on the C++ frontend would be a bit bigger, but not much. > >> > >> The patch contains a logically unrelated change: c-common.h set aside > >> 16 keywords for address spaces, but of the in-tree ports, the maximum > >> number of keywords used is 6 (for amdgcn). The patch therefore changes > >> the limit to 8 and uses 8 keywords for the new attributes. This keeps > >> the number of reserved ids <= 256. > > > > If you had added __arm(bar(args)) instead of __arm_bar(args) you would only > > need one additional keyword - we could set aside a similar one for each > > target then. I realize that double-nesting of arguments might prove a bit > > challenging but still. > > Yeah, that would work. > > > In any case I also think that attributes are what you want and their > > ugliness/issues are not worse than the ugliness/issues of the keyword > > approach IMHO. > > I guess the ugliness of keywords is the double underscore? > What are the issues with the keyword approach though?
The issue is the non-standard syntax which will confuse 3rd party tools like IDEs, static analyzers, etc. I'd also add that esp. my suggestion to use __arm will likely clash with pre-existing macros from (some) implementations. That can be solved with _ArmKWD or choosing a less "common" identifier. A quick check shows GCC on arm only defines __arm__, not __arm though. Richard. > If it's two underscores vs miscompilation then it's not obvious > that two underscores should lose. > > Richard
