aaron.ballman added a comment. In D134456#3827185 <https://reviews.llvm.org/D134456#3827185>, @dexonsmith wrote:
> In D134456#3827040 <https://reviews.llvm.org/D134456#3827040>, @aaron.ballman > wrote: > >> In D134456#3819185 <https://reviews.llvm.org/D134456#3819185>, @rnk wrote: >> >>> In D134456#3819042 <https://reviews.llvm.org/D134456#3819042>, >>> @aaron.ballman wrote: >>> >>>> Alternatively, we could document that we don't follow the intent of the >>>> standard feature and that's just the way things are, and add a command >>>> line option to honor that intent. However, given that GCC honors the >>>> attribute over PGO, I think we should change our default behavior. >>> >>> That makes sense to me. I think it's reasonable to request folks to add a >>> flag in the context of a bug fix, but it would be too much to ask a drive >>> by contributor to go through the multi-step process to change the default >>> flag behavior. >> >> I guess I view it differently -- I think users should get the correct >> behavior by default rather than have to opt into it, and I'm not certain >> that "it's a drive-by contribution" is a good driver of user experience >> decisions. Either way, I think we need an explicit decision as to which >> approach we think is *right* and go from there in terms of how to achieve >> that goal. (It'd be a bit different if I thought this patch was incremental >> progress, but from my current views on the topic, I actually think the patch >> is a further regression away from where we want to be.) >> >> I'm adding in @dexonsmith as "branch weights" code owner and @dnovillo as >> "SampleProfile and related parts of ProfileData" code owner on the LLVM side >> to see if they have opinions on default behaviors (if there are other PGO >> experts to bring in, please sign them up). > > The design of PGO was to use user hints when there’s no coverage, but > basically ignore them if theres enough data to say otherwise. Whelp, if that's the design, then I guess we should stick with it. It's rather unfortunate to me that our default behavior is "ignore what the user told us because we're certain we know better" when we know that to be false and isn't how GCC behaves in this case, but it also is what it is. At least it's consistent with other <tongue in cheek>highly successful standardized optimization hints</tongue in cheek> like `register`, `inline`, and `[[carries_dependency]]`. :-P > This safety scenario sounds like it could differ within a file. Is a flag > really the right control? Maybe what you want is a `[[noprofile]]`, similar > to `[[noopt]]`, which selectively disables the profile where the user wants > fine-grained control to ignore the profile. My understanding is that it's pretty rare for safety critical code to mix with non-safety critical code, so a flag sounds like the correct granularity to me in terms of the use cases I'm aware of. However, parallel attributes do give the most control to the user. My worry is that parallel attributes will be used as: #if __has_cpp_attribute(clang::likely_but_honor_this_one) #define LIKELY [[clang::likely_but_honor_this_one]] #elif __has_cpp_attribute(likely) #define LIKELY [[likely]] #else #define LIKELY #endif either due to need (like safety critical situations) or due to user confusion (like we used to see happen with `register` or `inline` in the Olden Days), but I suppose a command line flag runs that same danger of misuse and is about as portable, so I could go with either approach. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134456/new/ https://reviews.llvm.org/D134456 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
