[PATCH] D130807: [InstrProf] Add the omitprofile attribute
Paul Kirth via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 1 10:40:10 PDT 2022
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...
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits