[PATCH] D132186: Clang: Add a new flag Wmisnoinline for printing hot noinline functions

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

Thanks for taking a look!

In D132186#3752150 <https://reviews.llvm.org/D132186#3752150>, @paulkirth wrote:

> In D132186#3751985 <https://reviews.llvm.org/D132186#3751985>, @tejohnson wrote:
>
>> I have seen a few cases where noinline was used for performance, in addition to other cases like avoiding too much stack growth.
>
> Well, I stand corrected. I'm curious about what these cases are, but in any case if there are cases where its done, then I agree that a diagnostic would be helpful.

Same. The instances I've seen is an older codebase where compiler optimizations were not as powerful and/or purposefully written by engineers that didn't trust the compiler to do the right thing.

>> It is a little different than misexpect though in that the expect hints are pretty much only for performance, so it is more useful to be able to issue a strong warning that can be turned into an error if they are wrong. And also there was no way to report the misuse of expects earlier, unlike inlining where we already had the remarks plumbing.
>>
>> I haven't looked through the patch in detail, but is it possible to use your changes to emit a better missed opt remark from the inliner for these cases (I assume we will already emit a -Rpass-missed=inline for the noinline attribute case, just not highlighting that it is hot and would have been inlined for performance reasons otherwise)? I suppose one main reason for adding a warning is that the missed inline remarks can be really noisy and not really useful to the user vs a compiler optimization engineer doing inliner/compiler tuning, and therefore a warning would make it easier to turn on more widely as user feedback that can/should be addressed in user code.
>
> Yeah, I was thinking we could emit a new remark type for this to differentiate, but it seems simpler more user friendly to emit some clar diagnostic directly.
>
> I think we’re starting to accumulate a few of these diagnostics now that are trying to diagnose potential performance deficiencies based on profiling information. Originally we had prototyped a tool for misexpect based on libtooling that ran over the build based on the compile commands DB and reported everything it found.  I wonder if reviving that would be useful in these cases when you want to look for performance issues like this, misexpect, and other cases? Making ORE diagnostic output queryable through a tool may also be a good option, but I'm not too familiar with what already exists in that area.

Currently a new ORE (`-pass-remarks=misnoinline`) is getting generated, which misnoexcept also does. Agreed a warning is more familiar and friendlier for users so I lean towards that approach. For additional tooling, I think the first step will be to trial this on more real programs to see what cases are interesting. @iamarchit123 just finished his internship with us so I'll be evaluating these changes on HHVM to see if they can swing the performance needle.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186