Similarly to no_instrument_function for -finstrument-functions, some users wish that there be a function attribute to allow users to exclude a function from being instrumented with -fprofile-generate.
Not so easy though as profiling happens after inlining.
May I ask you Marek what would be use-case of such attribute? Thanks
E.g. if you have an application that is usually just busy waiting, waiting for something to happen, but you're not interested in profiling that function, or some function runs in a context where it is not safe to call into runtime support code.
I see. Simplest solution is to propagate the attribute to callers, or don't allow inlining between a pair of function one w/ and second w/o the attribute? Richi?
There is only early inlining happening before instrumentation. I suppose we could block early inliner for functions where attribute mismatches (it would need to block itself for both -fprofile-generate and -fprofile-use). problem is that if we late inline it, we will end up with function w/o profile within function with profile, but I plan to add better support for that anyway because it happens quite frequently for comdats (where profiling is lost) and for LTO where only some units has been profiled...
We had a request for something like this today on LKML, see the thread. https://lore.kernel.org/lkml/CAKwvOdmPTi93n2L0_yQkrzLdmpxzrOR7zggSzonyaw2PGshApw@mail.gmail.com/ And more specific use case: https://lore.kernel.org/lkml/20210614190700.GF68749@worktop.programming.kicks-ass.net/ I've implemented this in LLVM; no_instrument_function function attribute in C can be used to disable coverage of -fprofile-generate (instrumentation based profiling; "PGO") and -fprofile-arcs (coverage; "GCOV"). PGO: https://reviews.llvm.org/D104253 GCOV: https://reviews.llvm.org/D104257 Inlining is a good point and something I'll need to check; generally when caller's and callee's function attributes don't match, we block inline substitution (though we permit it for always_inline; developer be damned). One question Fangrui had made was whether no_instrument_function is the appropriate function attribute to re-use. https://reviews.llvm.org/D104253#2817695 It looks like both -finstrument-functions and -pg are affected by attribute no_instrument_function; I decided to reuse no_instrument_function in LLVM because: 1. it already exists; implementation is barely more than 1 LoC. 2. it already affects code gen of 2 different flags. 3. its name perfectly describes developer intent. 4. the Linux kernel is already wired up to make use of no_instrument_function attribute (though the kernel's configuration step (KConfig) will need changes to detect support for this issue probably). I haven't landed the changes in LLVM yet, and don't particularly care what the attribute used ultimately is even if that means revisting our approach in LLVM. But without a solution to this problem, it's likely to block PGO and regress GCOV for x86 Linux kernels.
Some notes. {gcc,clang} -fsanitize-coverage={trace-pc,trace-cmp} is another coverage feature. It uses no_sanitize_coverage instead of no_instrument_function. The GCC support for no_sanitize_coverage is very new (by Martin, in 2021-05-25). (In Clang, the feature has more modes, e.g. you can control func/bb/edge.) The Linux kernel use case (include/linux/compiler_types.h ) uses 'noinline' so inlining is not a concern. /* Section for code which can't be instrumented at all */ #define noinstr \ noinline notrace __attribute((__section__(".noinstr.text"))) \ __no_kcsan __no_sanitize_address Clang supports another filtering mechanism, -fprofile-list= (https://reviews.llvm.org/D94820). But the kernel use case seems to prefer a function attribute.
I am thinking of __attribute__((no_profile)). In Clang, -fprofile-generate(-fcs-profile-generate)/-fprofile-instr-generate/-fprofile-arcs are all different. It will make sense to have a attribute disabling all such profiling related features. I am not sure an umbrella __attribute__((no_instrument_function)) is suitable. The Linux kernel wanting noinstr to exclude -fprofile-* is a very specific characteristic, not suitable for other applications.
(In reply to Fangrui Song from comment #8) > I am thinking of __attribute__((no_profile)). > > In Clang, > -fprofile-generate(-fcs-profile-generate)/-fprofile-instr-generate/-fprofile- > arcs are all different. It will make sense to have a attribute disabling all > such profiling related features. > > I am not sure an umbrella __attribute__((no_instrument_function)) is > suitable. > The Linux kernel wanting noinstr to exclude -fprofile-* is a very specific > characteristic, not suitable for other applications. Sure, though now we'll need to add use of that to the Linux kernel. v2: https://reviews.llvm.org/D104475.
Link to kernel patches: https://lore.kernel.org/lkml/20210618233023.1360185-1-ndesaulniers@google.com/
Apparently, we already have an attribute that prevents function instrumentation regarding PGO: no_profile_instrument_function The no_profile_instrument_function attribute on functions is used to inform the compiler that it should not process any profile feedback based optimization code instrumentation. It's there for quite some releases. @Nick: Do you need anything more?
Ah, perfect! commit 1225d6b1134b ("Introduce no_profile_instrument_function attribute") LGTM: https://godbolt.org/z/779xzndY6 Looks like it landed in GCC 7.1. Let me change over the attribute identifier in Clang to match, then I'll resend the kernel patches, and close this out.
What's likely missing is that the attribute should prevent inlining. I'm going to test how it behaves right now. Then, the issue can be closed.
(In reply to Martin Liška from comment #13) > What's likely missing is that the attribute should prevent inlining. I'm > going to test how it behaves right now. Then, the issue can be closed. It's not clear to me that no_profile_instrument_function should prevent inlining. I'll argue that attributes should be orthogonal. https://lists.llvm.org/pipermail/llvm-dev/2021-April/150062.html https://reviews.llvm.org/D101011#2715555 If the user wants to suppress inlining, add noinline. Can a no_profile_instrument_function function be inlined to another no_profile_instrument_function function? Why not. Can a no_profile_instrument_function function be inlined into a function without the attribute? This may be controversial but I'd argue that it can. GCC no_stack_protector behaves this way. no_profile_instrument_function can mean that user does not want profiling when the function is called with its entity, not via another entity.
(In reply to Fangrui Song from comment #14) > Can a no_profile_instrument_function function be inlined into a function > without the attribute? This may be controversial but I'd argue that it can. > GCC no_stack_protector behaves this way. no_profile_instrument_function can > mean that user does not want profiling when the function is called with its > entity, not via another entity. I respectfully but strongly disagree. It's surprising to developers when they ask for no stack protector, or no profiling instrumentation, then get one anyways. For long call chains, it's hard for developers to diagnose on their own which function they called that missed such function attribute. This reminds me of "what color is your function?" https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/ As suddenly a developer would need to verify for a no_* attributed function that they only call no_* attributed functions, or add noinline (which is a big hammer to all call sites, and games with aliases that have the noinline attribute are kind of ridiculous). It's less surprising to prevent inline substitution upon function attribute mismatch. Then a developer can self diagnose with -Rpass=inline. Either way, some form of diagnostics would be helpful for these kinds of issues, and has been requested by Android platform developers working on Zygote. For no_stack_protector in LLVM, I implemented the rules: upon mismatch, prevent inline substitution unless the user specified always_inline. This fixed suspend/resume bugs in x86 Linux kernels when built with LTO. Though, I'm happy to revisit that behavior in LLVM; we could add #define noinline_for_lto __attribute__((__noinline__)) then use that in the Linux kernel instead.
Clang patch (no_profile -> no_profile_instrument_function): https://reviews.llvm.org/D104658 Kernel patches v2: https://lore.kernel.org/lkml/20210621231822.2848305-1-ndesaulniers@google.com/
All right, similarly to sanitizer flags, I sent a patch that prevent inlining when -fprofile-generate is used: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573511.html Note that one typically uses the attribute when a function is hot and would delay instrumentation a lot. That's why we don't want the function be inlined. Moreover, each hot function excluded from instrumentation should be likely decorated with 'hot' attribute.
(In reply to Nick Desaulniers from comment #15) > (In reply to Fangrui Song from comment #14) > > Can a no_profile_instrument_function function be inlined into a function > > without the attribute? This may be controversial but I'd argue that it can. > > GCC no_stack_protector behaves this way. no_profile_instrument_function can > > mean that user does not want profiling when the function is called with its > > entity, not via another entity. > > I respectfully but strongly disagree. It's surprising to developers when > they ask for no stack protector, or no profiling instrumentation, then get > one anyways. For long call chains, it's hard for developers to diagnose on > their own which function they called that missed such function attribute. > > This reminds me of "what color is your function?" > https://journal.stuffwithstuff.com/2015/02/01/what-color-is-your-function/ > As suddenly a developer would need to verify for a no_* attributed function > that they only call no_* attributed functions, or add noinline (which is a > big hammer to all call sites, and games with aliases that have the noinline > attribute are kind of ridiculous). > > It's less surprising to prevent inline substitution upon function attribute > mismatch. Then a developer can self diagnose with -Rpass=inline. Either way, > some form of diagnostics would be helpful for these kinds of issues, and has > been requested by Android platform developers working on Zygote. > > For no_stack_protector in LLVM, I implemented the rules: upon mismatch, > prevent inline substitution unless the user specified always_inline. This > fixed suspend/resume bugs in x86 Linux kernels when built with LTO. > > Though, I'm happy to revisit that behavior in LLVM; we could add > > #define noinline_for_lto __attribute__((__noinline__)) > > then use that in the Linux kernel instead. Our problem is that a boolean attribute with 1 bit information cannot express whether a neg attribute function can be inlined into a pos attribute function. Let's agree to disagree. I don't see why a no_profile_instrument_function function suppress inlining into a function without the attribute. For the use cases where users want to suppress inlining, they can add noinline. What I worry about is that now GCC has an attitude and if the LLVM side doesn't follow it is like diverging. However, the GCC patch is still in review. I think a similar topic may need to be raided on llvm-dev side as I feel this is the tip of the iceberg - more attributes can be similarly leveraged. So, how about a llvm-dev discussion?
(In reply to Fangrui Song from comment #18) [...] > Our problem is that a boolean attribute with 1 bit information cannot > express whether a neg attribute function can be inlined into a pos attribute > function. > > Let's agree to disagree. I don't see why a no_profile_instrument_function > function suppress inlining into a function without the attribute. For the > use cases where users want to suppress inlining, they can add noinline. What > I worry about is that now GCC has an attitude and if the LLVM side doesn't > follow it is like diverging. However, the GCC patch is still in review. I > think a similar topic may need to be raided on llvm-dev side as I feel this > is the tip of the iceberg - more attributes can be similarly leveraged. So, > how about a llvm-dev discussion? I have mentioned this several times now, but it seems nobody is listening: It's _not_ about inlining -- the inlining discussion is about a particular implementation strategy. It's about the _contract an attribute promises_, which is treating the code in the function a certain way (e.g. do not instrument). That can be done by either: a) even if the code is inlined, respect the original attribute for the inlined code (e.g. do not instrument), or b) just don't inline. It looks like (b) is easier to do. I probably do not understand how hard (a) is. If you break the contract because it's too hard to do (a), then that's your problem. Just don't break the contract. Because that's how we get impossible-to-diagnose bugs. Correctness comes first: if it is impossible for a user to reason about the behaviour of their code due to unspecified behaviour (viz. breaking the contract) of an attribute, then the code is doomed to be incorrect. Therefore, do _not_ implement attributes with either unspecified or ridiculously specified behaviour. Ridiculous in this case is saying "this attribute only does what it promises if you also add noinline". It's ridiculous, because the user will then rightfully wonder "?!?!? Why doesn't it imply noinline then?!?!?!". Thanks.
(In reply to Marco Elver from comment #19) I am ok with "inlining suppression" as an implementation strategy and I agree that it should be useful. What I objected strongly is "promised inlining suppression". For example, if an inlining pass happens after instrumentation, then the function attribute doesn't necessarily need to suppress inlining. After instrumentation is done, we can even treat the noprofile attribute as a no-op. The example applies to the non-LTO case -fsanitize-coverage= . (We don't actually use the noprofile function attribute for -fsanitize-coverage=, but I cannot find a better example in LLVM; I think all other noprofile affected instrumentations happen before the inliner pipeline). So in a documentation, it can be said that the inlined copy (if any) will not get instrumentation, but it **should not** say that a noprofile function cannot be inlined into a function without the attribute.
(In reply to Fangrui Song from comment #20) > For example, if an inlining pass happens after instrumentation, then the > function attribute doesn't necessarily need to suppress inlining. After > instrumentation is done, we can even treat the noprofile attribute as a > no-op. Sent too early:) Amendment: a smart inliner can inline the noprofile callee and then drop instrumentation code. That will also be an approach which does not break the "no instrumenting my code" contract. Other approaches can be (probably more relevant to function specialization/clones): the instrumentation pass can leave an un-instrumented copy which can be used by a subsequent inliner. As we can see, all these approaches are much more complex than simply "suppressing inlining". So I agree that "suppressing inlining" is a good implementation detail here.
> For example, if an inlining pass happens after instrumentation, then the > function attribute doesn't necessarily need to suppress inlining. After > instrumentation is done, we can even treat the noprofile attribute as a > no-op. In the case of GCC, the instrumentation happens before inlining, but after early inlining. The patch reviewer noticed that and we would implement it that way. So yes, in order to preserve the attribute contract, we have to block inlining for functions decorated with the attribute.
(In reply to Fangrui Song from comment #18) > I > think a similar topic may need to be raided on llvm-dev side as I feel this > is the tip of the iceberg - more attributes can be similarly leveraged. So, > how about a llvm-dev discussion? Sure. llvm-dev RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-June/151509.html llvm patch: https://reviews.llvm.org/D104810
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>: https://gcc.gnu.org/g:aad72d2ea8378e1a56c00d15daa4bdcac8a5ae39 commit r12-3379-gaad72d2ea8378e1a56c00d15daa4bdcac8a5ae39 Author: Martin Liska <mliska@suse.cz> Date: Tue Jun 22 10:09:01 2021 +0200 inline: do not einline when no_profile_instrument_function is different PR gcov-profile/80223 gcc/ChangeLog: * ipa-inline.c (can_inline_edge_p): Similarly to sanitizer options, do not inline when no_profile_instrument_function attributes are different in early inliner. It's fine to inline it after PGO instrumentation. gcc/testsuite/ChangeLog: * gcc.dg/no_profile_instrument_function-attr-2.c: New test.
Should be implemented now.