aaron.ballman marked an inline comment as done. aaron.ballman added a comment.
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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143670/new/ https://reviews.llvm.org/D143670 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
