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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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. 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.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143670