[PATCH] D143670: Stop claiming we support [[carries_dependency]]

Erich Keane via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 21 11:14:56 PST 2023


erichkeane added a comment.

In D143670#4142351 <https://reviews.llvm.org/D143670#4142351>, @aaron.ballman wrote:

> In D143670#4133677 <https://reviews.llvm.org/D143670#4133677>, @rsmith wrote:
>
>> Until or unless a C++ DR permits us to define `__has_cpp_attribute(carries_dependency)` to any value other than `200809L`, we have a conformance requirement to macro-expand this to that value. CWG2695 only adds a note, so it changes nothing in this regard.
>
> We don't yet have a C++ DR for this, but we do have EWG direction that such a DR is needed. At the end of the day, our behavior in Clang is inconsistent and this patch fixes that inconsistency. `__has_cpp_attribute(no_unique_address)` returns `0` on some targets for Clang already today. That cannot change without the risk of silently breaking ABI for users. This is why EWG is moving in the direction of making the value returned by the feature test macro be a matter of QoI. So in terms of behaving consistently, we're faced with a choice: potentially break users by returning nonzero for `no_unique_address` or stop returning nonzero for `carries_dependency`. GCC has always returned 0 for `carries_dependency`. Returning 0 makes Clang consistent with GCC for this specific attribute and makes Clang consistent with itself in terms of standard attributes.
>
>> Similarly, we have a conformance requirement to emit a diagnostic on invalid uses of the attribute, and per our policy for `-pedantic`, we should produce an error for invalid uses under `-pedantic-errors` and not reject valid uses in that mode. So I think we still need all of the functionality that this patch removes for conformance reasons, for now at least. Maybe WG21 would be receptive to a paper making support of `[[carries_dependency]]` optional.
>
> The behavior of `-pedantic-errors` is an interesting point. We *conform* with an "attribute ignored" diagnostic as far as the standard is concerned (so there's no problem about conformance requirements) but it doesn't meet our usual behavior of giving an error in `-pedantic-errors` mode. We have quite a few other instances of missing pedantic errors and it'd be nice to not make that worse. However, given that we are missing plenty of other pedantic error diagnostics, I'm not certain that observation changes a whole lot in practice.
>
>> Conformance aside, the meaning of `[[carries_dependency]]` on a particular platform is an ABI requirement. On every platform that we support, the psABI does not specify any different handling for `[[carries_dependency]]` functions, so we *are* implementing the attribute completely and correctly on those targets, it just happens to be vacuous. If there were a target that defined a different ABI rule, or was anticipated to add such a different ABI rule (eg, due to some future PowerPC or ARM ABI change), then it would make sense to stop claiming support on those platforms until we implemented the behavior (I have the behavior of `[[no_unique_address]]` on MS ABI in mind here particularly). But I think it's not helpful to users to say we don't support it because it doesn't happen to make a difference on the current target, just it's not helpful to say we don't support `[[no_unique_address]]` just because it doesn't affect struct layout on a particular target, or -- more to the point -- it wouldn't be helpful to say we don't support `consume` memory ordering just because we don't get any benefit from it on the current target compared to `acquire` (and as a result, we lower `consume` to `acquire`). I think it's much better to let people unconditionally add `[[carries_dependency]]` (or `[[no_unique_address]]`, or to use `consume`) when they mean it; that way, if they compile their code on both Clang and some other compiler that supports a target where the attribute does something, then they'll get the behavior they asked for. We shouldn't be encouraging people to wrap uses of standard attributes in macros.
>
> Errr, I have exactly the opposite opinion as you on this. I do not think it helps users to claim vacuous support for an attribute -- that's a response only a compiler writer/committee member could appreciate. In my experience, users want to know "does the attribute do what it says on the tin" not "can the compiler trivially claim conformance". `[[carries_dependency]]` is an optimization hint; that we don't attempt to even pass the information along to the backend to try to vary optimization behavior means we're not doing what the attribute says on the tin. And given that standard attributes are inherently not portable nor backwards compatible, I think it's very reasonable to encourage people to wrap their use in macros.
>
> Otherwise, what's the point to `__has_cpp_attribute` returning *anything* for a standard attribute? Users can check `__cplusplus` instead if the only answer they need to get from the feature test macro is "what version of the standard are you using?".
>
>> That said, even though I think we should claim (vacuous) support for such cases, I think it would be reasonable to warn developers that they have no effect. For example, we could add a special-case sub-group of `-Wignored-attribute` to tell developers that "carries_dependency attribute has no effect", because they might be combining it with `consume` and expecting improved performance. It's probably even reasonable to turn such a warning on by default.
>
> I continue to believe the best approach here is the honest approach. We're not obligated to implement this attribute, we don't currently implement this attribute in any meaningful way, and so the best approach is to handle it the same as any other attribute we don't know or care about.

The last bit (intent of `__has_cpp_attribute` being that we 'do something useful with it') was the intent of the room in EWG.  Note the term 'useful' was not used in the polls thanks to some CWG folks invading and telling us that 'useful' had meaning apart from what we meant :)  That said, the subtlety of "we do exactly what you expect, which is NOTHING!" for carries-dependency wasn't brought up in the room.

It is ALSO the vote taken in EWG to make setting the value optional, see the poll "Ask CWG to change the definition of __has_cpp_attribute to permit 0, and place the suggested values in the 'recommended practice' of the individual attributes.", which gained consensus (https://github.com/cplusplus/papers/issues/1212).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143670/new/

https://reviews.llvm.org/D143670



More information about the cfe-commits mailing list