ellis added a comment. In D130807#3688798 <https://reviews.llvm.org/D130807#3688798>, @paulkirth wrote:
> Do you expect the difference between `noprofile` and `omitprofile` to be > confusing to users? I can certainly see how users could be confused by what > the difference is between `noprofile` and `omitprofile` ... > > Since what you want to communicate is "never profile"(which `noprofile` can > probably communicate as is) and "never profile this, but allow inlining of > profiled functions" (which I'm not sure `omitprofile` communicates), then > maybe there is a more obvious way we could communicate that? Maybe > `neverprofile` and `nodirectprofile` are more descriptive names? I don't love > the idea of changing an existing attribute name, but we can transparently > upgrade existing uses for backwards compatibility if we have to. What do you > think? I agree that `omitprofile` probably doesn't communicate the correct meaning very well. I like `nodirectprofile` because it hopefully implies that indirect profiles are allowed. I'm wondering if anyone else has suggestions. Is there any precedent for renaming function attributes? My assumption was that users would add the `noprofile` attribute to source mostly for correctness concerns and `noprofile` is easy to remember and already in use, so I'd rather not change it. > I also think this will need to need to be supported in LLVM's passes, right? > So the instrumentation passes in `PGOInstrumentation.cpp` (I can't remember > if 'InstrProfiling.cpp` will need this too), and probably the Inlining passes > too, right? I assume those will be follow up patches, but I don't see them > in the current stack. Is that an accurate assumption? I actually do have a change in `PGOInstrumentation.cpp` that skips profiling functions with this new attribute. As far as I know, this stack has all the necessary code changes. > BTW I like the direction of these patches, I think adding the ability to > differentiate these cases will add a lot of value to the profiling runtimes + > coverage. :) Thanks for the suggestion and feedback! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130807/new/ https://reviews.llvm.org/D130807 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
