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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.



> 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 agree with you that it's probably best to avoid changing the spelling of `noprofile`, but since you're making a change in the area, we may as well discuss if 1) that is still the best name, and 2) if we want to keep it.

I don't know if we've ever changed the spelling of a `FnAttribute` before. IIRC we've dropped them, and changed their format e.g., from being a boolean to taking a value or an enum. At least that's my memory, which seems to correspond to the autoupgrade stuff in `BitcodeReader`. But I think the project's stance on this(like most things) is that "if there's a good reason, then we should do it." At least if we can maintain our stated compatibility guarantees. This is a place where I think we should get additional feedback before proceeding though.

It might be worth asking about this on discourse. Even if we don't want to make such a change in these patches, it may be good for the project to have a policy (or at least guidelines) about changing attributes.

> 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.

I can't believe I missed that. I looked three times before I commented, and blew right passed it every time. That's what happens when I look at code late on a Friday...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130807/new/

https://reviews.llvm.org/D130807