aaron.ballman accepted this revision. aaron.ballman added a comment. Only thing left for me are the nits I pointed out in the last review, otherwise I think this is all set. Giving my LG because I don't think we need another round of review for those nits unless you want it
================ Comment at: clang/lib/Sema/ParsedAttr.cpp:241 + case AT_ObjCGC: + case AT_VectorSize: + return true; ---------------- rsmith wrote: > mboehme wrote: > > rsmith wrote: > > > This one is weird. Given: > > > > > > ``` > > > [[gnu::vector_size(8)]] int x; > > > int [[gnu::vector_size(8)]] y; > > > int *[[gnu::vector_size(8)]]* z; > > > ``` > > > > > > - Clang and GCC accept `x` > > > - Clang rejects `y` and GCC warns that the attribute is ignored on `y` > > > - Clang accepts `z` with a warning that GCC would ignore the attribute, > > > whereas GCC silently accepts > > > > > > I guess after this patch we'll silently accept `x` (treated as the > > > "sliding" behavior) and accept `y` and `z` with a warning that it's > > > GCC-incompatible? > > > This one is weird. Given: > > > > > > ``` > > > [[gnu::vector_size(8)]] int x; > > > int [[gnu::vector_size(8)]] y; > > > int *[[gnu::vector_size(8)]]* z; > > > ``` > > > > > > - Clang and GCC accept `x` > > > - Clang rejects `y` and GCC warns that the attribute is ignored on `y` > > > - Clang accepts `z` with a warning that GCC would ignore the attribute, > > > whereas GCC silently accepts > > > > Actually, for z, Clang gives me not just a warning but also an error: > > > > ``` > > <source>:1:8: warning: GCC does not allow the 'vector_size' attribute to be > > written on a type [-Wgcc-compat] > > int *[[gnu::vector_size(8)]]* z; > > ^ > > <source>:1:8: error: invalid vector element type 'int *' > > ``` > > > > https://godbolt.org/z/E4ss8rzWE > > > > > I guess after this patch we'll silently accept `x` (treated as the > > > "sliding" behavior) and accept `y` and `z` with a warning that it's > > > GCC-incompatible? > > > > Thanks for pointing this out to me! > > > > Actually, I must not have checked the GCC behavior previously, so I went > > too far and gave `vector_size` meaning when placed on `y` instead of just > > ignoring it. I’ve now changed the code to ignore the attribute and emit a > > warning, like GCC does. (See tests in `vector-gcc-compat.c`.) > > > > It’s particularly surprising to me that GCC allows `vector_size` to be > > applied to a pointer type (the `z` case) – particularly so since the > > [documentation](https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html) > > explicitly states: “The vector_size attribute is only applicable to > > integral and floating scalars, although arrays, pointers, and function > > return values are allowed in conjunction with this construct.” > > > > I wanted to test whether GCC just silently ignores the attribute when > > placed on a pointer or whether it has an effect. Here are some test cases: > > > > https://godbolt.org/z/Ke1E7TnYe > > > > From looking at the generated code, it appears that `vector_size`, when > > applied to a pointer type, _does_ have an effect – though a maybe slightly > > surprising one: It is applied to the base type, rather than the pointer > > type. I suspect that GCC simply treats the C++11 spelling the same as the > > `__attribute__` spelling (i.e. it doesn’t apply the stricter C++11 rules on > > what the attribute should appertain to), and it seems that both spellings > > get applied to the base type of the declaration no matter where they appear > > in the declaration (except if the attribute is ignored entirely). > > > > Interestingly, this means that Clang’s warning (“GCC does not allow the > > 'vector_size' attribute to be written on a type”) is wrong. Maybe this is > > behavior in GCC that has changed since the warning was introduced? > > > > Given all of this, I propose we treat the various cases as follows (and > > I’ve implemented this in the code): > > > > * We continue to accept `x` without a warning, just as we do today (same > > behavior as GCC) > > * We allow `y` but warn that the attribute doesn’t have an effect (same > > behavior as GCC) > > * We continue to reject `z`, even though GCC allows it and it has an effect > > there. Rationale: a) We reject this today, so this isn’t a regression; b) > > this usage is unusual and likely not to occur often in the wild; c) fixing > > this to be consistent with GCC will take a non-trivial amount of code, so > > I’d like to keep it outside the scope of this change. > > https://godbolt.org/z/Ke1E7TnYe > > That GCC behavior is shocking. Shocking enough that I think the following > approach would make sense: > > 1) For compatibility, emulate the GCC behavior as much as possible for > `[[gnu::vector_size]]`, except: > - Continue to error rather than only warning in the cases where the > attribute does not create a vector type at all, and > - Warn on cases like `int *[[gnu::vector_size(N)]]` (and possibly `int *p > [[gnu::vector_size(N)]]`) that we're forming a pointer to a vector rather > than a vector of pointers. > 2) Add a separate `[[clang::vector_size(N)]]` attribute that behaves like a > type attribute is supposed to: the attribute applies to the type on its left, > always (and can't be applied to a declaration). > > (Incidentally, I think we should add a `[[clang::ext_vector_type]]` spelling > with behavior (2) too. The .td file claim that ext_vector_type is > OpenCL-specific isn't true; it's a general kind of vector type that follows > the OpenCL rules but is available across language modes.) > > But... not in this patch. In this patch I think the only priority for the > `[[gnu::...]]` attributes is that we don't make things worse (in particular, > don't make GCC compatibility worse). And given that this new sliding behavior > applies only to `[[clang::...]]` attributes I think we have that covered. So > no action required here. +1 to this :-) ================ Comment at: clang/lib/Sema/ParsedAttr.cpp:224 + // property is consciously not defined as a flag in Attr.td because we don't + // want new attributes to specify it. + switch (getParsedKind()) { ---------------- rsmith wrote: > aaron.ballman wrote: > > mboehme wrote: > > > aaron.ballman wrote: > > > > mboehme wrote: > > > > > aaron.ballman wrote: > > > > > > Thoughts on the additional comment? And should we start to issue > > > > > > that deprecation warning now (perhaps as a separate follow-up > > > > > > commit)? > > > > > > Thoughts on the additional comment? > > > > > > > > > > Yes, good point, we should point this out explicitly. > > > > > > > > > > I've worded the comment slightly differently: we can definitely > > > > > deprecate the "sliding" behavior for attributes in the `clang::` > > > > > namespace, as we own them, but there may be compatibility concerns > > > > > for other attributes. This may mean that we can never reduce this > > > > > list to nothing, but we should try. > > > > > > > > > > > And should we start to issue that deprecation warning now (perhaps > > > > > > as a separate follow-up commit)? > > > > > > > > > > We're already doing this (see the tests in > > > > > test/SemaCXX/adress-space-placement.cpp), though again only for > > > > > attributes in the `clang::` namespace, as we don't really have the > > > > > authority to deprecate this usage for other attributes. > > > > > > > > > > I think the other question here is whether, by default, this should > > > > > be an error, not a warning, but this would then presumably require a > > > > > command-line flag to downgrade the error to a warning if needed (I > > > > > believe @rsmith raised this point on > > > > > https://reviews.llvm.org/D111548). If we do decide to make this an > > > > > error by default, I'd like to do this in a followup change if > > > > > possible, as a) this change is already a strict improvement over the > > > > > status quo; b) adding a command-line flag with associated tests would > > > > > further increase the scope of this change, and c) this change is a > > > > > blocker for https://reviews.llvm.org/D111548, which others on my team > > > > > are waiting to be able to use. > > > > > ... but there may be compatibility concerns for other attributes. > > > > > This may mean that we can never reduce this list to nothing, but we > > > > > should try. > > > > > > > > I would be pretty aggressive there and break compatibility over this, > > > > unless the uses were in system headers or something where the break > > > > would be too onerous to justify. > > > > > > > > > I think the other question here is whether, by default, this should > > > > > be an error, not a warning, but this would then presumably require a > > > > > command-line flag to downgrade the error to a warning if needed > > > > > > > > You put `DefaultError` onto the warning diagnostic to have it > > > > automatically upgraded as if the user did `-Werror=whatever-warning`, > > > > which allows the user to downgrade it back into a warning with > > > > `-Wno-error=whatever-warning`. > > > > > I think the other question here is whether, by default, this should > > > > > be an error, not a warning, but this would then presumably require a > > > > > command-line flag to downgrade the error to a warning if needed > > > > > > > > You put `DefaultError` onto the warning diagnostic to have it > > > > automatically upgraded as if the user did `-Werror=whatever-warning`, > > > > which allows the user to downgrade it back into a warning with > > > > `-Wno-error=whatever-warning`. > > > > > > Oh nice -- I didn't realize it was this easy! > > > > > > I haven't made a change yet because I wanted to give @rsmith an > > > opportunity to weigh in -- do you have an opinion on whether this should > > > be a warning or an error by default? > > I'm not certain if @rsmith has opinions here or not, but I think an error > > which can be downgraded to a warning is a reasonable approach. We should > > hear pretty quickly if this causes significant problems (in system headers > > or the like). > I think I'd prefer for this to be a warning in the initial release with the > new behavior, and for us to upgrade it to an error for a subsequent release. > I'm not completely comfortable with there being no `[[clang::...]]` placement > for these attributes that is accepted by default in both Clang 14 and Clang > 15, but somewhat more comfortable with there being no syntax that's accepted > in both Clang 14 and Clang 16 by default, even though I think these > attributes are in practice mostly used with `__attribute__((...))` syntax. Okay, that's convincing, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126061/new/ https://reviews.llvm.org/D126061 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
