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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added a comment.

In D132186#3752403 <https://reviews.llvm.org/D132186#3752403>, @modimo wrote:

> 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.

Thanks for pointing that out. I had failed to consider those scenarios. I do recall having discussions w/ hardware/embedded engineers a long time ago regarding their mistrust of the compiler, so I should have thought about these types of situations.

> 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.

@iamarchit123, thanks for your contribution, and I hope your summer went well!  You may want to see if you can present your work at the dev meeting in November.  Even a lighting talk is a great item for a resume. There's even a student travel grant to help out w/ costs. https://discourse.llvm.org/t/llvm-foundation-student-travel-grants-available-sept-8-deadline/64794. Also, universities often have their own travel grants for students presenting at a conference, so I would encourage you to see if you're eligible for one of those from your own university too.

@modimo Keep us posted w/ your findings from HHVM, it will be interesting to see what kind of improvements can be gained.



================
Comment at: llvm/docs/MisNoInline.rst:2
+===================
+Misnoexpect
+===================
----------------



================
Comment at: llvm/docs/MisNoInline.rst:9-39
+When developers use noinline attribute with a function, i.e., through use of
+``__attribute__((noinline))``, they are trying to communicate that the
+function should not be inlined by the compiler. These can be due to various
+number of reasons to mark a function noinline. If a function is small,
+not critical to the performance of your code and would be called less often,
+especially in cases of error handling, it makes sense to noinline a function.
+These annotations, however, can be incorrect for a variety of reasons:
----------------
This is almost a verbatim copy of the MisExpect.rst. If these diagnostics are so similar, and use the exact same methodology, then maybe we should unify them as something like `Profile-based Mis-annotation Analysis`? We can describe the basic approach and then in subsections describe how individual diagnostics work/differ. What do you think?


================
Comment at: llvm/include/llvm/Analysis/InlineCost.h:174
   const char *Message = nullptr;
+  bool IsNoInline = false;
   InlineResult(const char *Message = nullptr) : Message(Message) {}
----------------
Do you need this variable? you store it here in the InlineCost, but the only place its used is in `canEmitNoInlineWarning`. Seems easier to just check for `Attribute::NoInline` on the `Callee` variable directly in `canEmitNoInlineWarning`, and avoid plumbing all this data around.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:69-71
+    cl::desc("Use this option to turn on/off "
+             "warnings about usage of noinline function attribute "
+             "which may hurt performance."));
----------------
the verbage here makes `noinline` sound like its always bad/wrong. Maybe "... noinlne function attribute on hot functions, which may degrade performance"?


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:148-150
+  Function &Callee = *CB->getCalledFunction();
+  LLVMContext &Ctx = Callee.getContext();
+  const char *reason = IC.getReason();
----------------
nit: Maybe sink these until they're needed?


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:163
+    return false;
+  if (!IC.isNoInline())
+    return false;
----------------
why not check for `NoInline` on either `CB` or `Callee` directly?


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:166-169
+  if (MisNoInlinePercent)
+    Percent_Threshold = MisNoInlinePercent;
+  else
+    Percent_Threshold = Ctx.getDiagnosticsMisNoInlinePercentileThreshold();
----------------
Seems like a helper function would make this a little cleaner/ergonomic.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:233-242
+    if (LLVM_UNLIKELY(canEmitNoInlineWarning(&CB, IC, FAM))) {
+      Function &Callee = *CB.getCalledFunction();
+      auto &CalleeTTI = FAM.getResult<TargetIRAnalysis>(Callee);
+      auto TempIC = llvm::isInlineViableFromCostAnalysis(
+          CB, &Callee, Params, CalleeTTI, GetAssumptionCache, GetTLI, GetBFI,
+          PSI);
+
----------------
Can we separate this out into its own function? or maybe it makes more sense to be part of `getInlineCost`? If it were there, then you may avoid invoking `isInlineViableFromCostAnalysis` more than necessary, right?

also, neither `Callee` nor `CalleeTTI` have changed from their def above, so do we need to shadow them here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132186