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

Thu Aug 25 14:37:10 PDT 2022

iamarchit123 added a comment.

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

> Hi, thanks for taking a look at this. Before we start an in-depth review, can you describe the deficiencies w/ the existing diagnostics, and why they don't meet your needs?
>
> Primarily, I'm a little skeptical if taking the same approach as MisExpect is the correct approach.
>
> Unlike `llvm.expect`, the `noinline` attribute is often used for correctness. I'm not sure it makes sense to warn about it in the same way as performance optimization. My experience may differ from the code bases you work in, but I cannot recall seeing a function annotated `noinline` for due to any kind of performance reason. The one exception I can think of being code marked as `cold` for outlining purposes, but those are usually inferred from profiles or are added due to other annotations. Do people do this for better i-cache performance or something?

I was under the impression that it was used for performance reasons and hot functions should not ideally be marked noinline. @modimo could you also pitch in on this? If not function entry count what other things could possibly indicate a false usage of noinline(if not the hotness)

> MisExpect diagnostics need to run a specific points during compilation to check the weights added by the `llvm.expect` intrinsic against the profile, so it can't be a separate pass, since e.g., LowerExpectIntrinsicPass and the PGO passes for instrumentation/sampling replace/remove that information. From what I can see this could be its own analysis pass, since you at most need to consult the function entry count.

This can be a separate pass but even I am using Cost Analysis to check noinline functions cost apart from function entry count, so it felt natural to integrate the noinline warning into inline cost analysis.

> Optimization remarks already exist for missed inlining opportunities. I'm unsure of the value in using a warning over the existing diagnostic in this case. In the case of `llvm.expect` intrinsics, it may be the result of an incorrect annotation, or a mis-annotated branch (i.e., marking a branch w/ `LIKELY` instead of `UNLIKELY`). In these cases, we'd like to signal to users a problem w/ the source code/annotation. I'm not sure that the same is true for `noinline` attributes. Is this something you want to use to fail builds? That was something we wanted to achieve for Fuchsia's CI, which is why `-Wmisexpect` exists as more than an optimization remark.

I think there is no Optimization remark emission for missed inlining opportunities for noinline functions as cost analysis is skipped for functions marked with noinline attribute. No we don't want any builds to fail with this. Our only aim to make this change is to emit a warning for functions which a user may have accidentally marked noinline but removing noinline attribute may give some performance benefits.

@modimo feel free to pitch in as well on this about concerns raised by @paulkirth and any changes I could make on this based on the comments.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186