[PATCH] D130807: [InstrProf] Add the omitprofile attribute

Mon Aug 1 10:10:11 PDT 2022

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