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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
