aaron.ballman added a comment. In D134456#3827579 <https://reviews.llvm.org/D134456#3827579>, @dexonsmith wrote:
> In D134456#3827478 <https://reviews.llvm.org/D134456#3827478>, @aaron.ballman > wrote: > >> In D134456#3827410 <https://reviews.llvm.org/D134456#3827410>, @dexonsmith >> wrote: >> >>> In D134456#3827377 <https://reviews.llvm.org/D134456#3827377>, >>> @aaron.ballman wrote: >>> >>>> Another thought I had is: when PGO is enabled, can we diagnose when PGO >>>> countermands an explicit annotation? Something to at least alert the user >>>> "hey, look over here, this suggestion was wrong" so they have more of a >>>> chance of knowing something is up? >>> >>> Seems pretty noisy, much like when the inliner countermands an `inline` >>> hint. Maybe an optimization remark would be appropriate, allowing advanced >>> users to selectively enable them in a critical section when debugging a >>> performance issue? >> >> Hmmm, on the one hand, any optimization hint is really an advanced user >> feature and so the opt remark might make sense. On the other hand, when I >> posted about `[[likely]]` and PGO on Twitter (not exactly what I'd call a >> scientific poll, take with an ocean of salt!), multiple people were >> surprised and expected PGO to honor the attribute, which suggests users may >> want this information. I don't have a good feel for what the user >> expectations are here, so I think an opt remark would be the most >> conservative way to start, and if we find that it's too hidden and >> non-expert users really need warnings, we can add a warning at that point. >> WDYT? > > Let's say it *was* scientific: even then, I disagree with your assertion that > users *really* want this information, just because they were surprised about > what happens. Many users will be surprised when told that the optimizer > reorders their statements; or, more relevantly, that adding `inline` doesn't > cause a function to be inlined; it doesn't follow that we should diagnose > those situations. > > I think what users want is good performance, and a way to fix the performance > when it's poor. If optimization remarks aren't serving the purpose of the > latter (which I believe was their design goal), maybe they can be improved > somehow. +1; I think we agree on what users want. I have no idea if opt remarks aren't serving their purpose, it's possible they're perfectly fine. > Optimization annotations are generally annoying for users since they give the > illusion of control, and users feel frustrated when they aren't followed. > Adding a diagnostic sounds nice, since then "at least they have some > visibility"... but in practice the optimizer supersedes user expectations > *everywhere*; you'll find it challenging to enable such diagnostics without > getting back a wave of non-actionable diagnostics. IMO, `[[likely]]` isn't > special here. > > Another point: having a diagnostic fire (failing a `-Werror` build) depending > on the content of the profile passed to `-fprofile-use` seems pretty hostile > to build workflows. Okay, this last point is especially compelling to me. I think opt remarks are probably the correct way to go. Thank you (everyone!) for the discussion on this. To make sure we're all on the same page for where we're at: 1. The changes in this review are reasonable and once review is finished, we're clear to land it. 2. We should file an issue to track the feature request for adding opt remarks for likelihood attribute disagreements. 3. We should file a bug to improve the PGO documentation (https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization) to explicitly document our behavior around explicitly-provided user annotations that disagree with PGO (this goes beyond `[[likely]]` and into things like `__builtin_expect`, `inline`, etc). Does that sound like the right plan to others? 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
