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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

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.

1. 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 a 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?
2. 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.
3. 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186