[PATCH] D110891: [inliner] Mandatory inlining decisions produce remarks

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 3 inline comments as done.
mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:52
 
+namespace {
+using namespace llvm::ore;
----------------
wenlei wrote:
> mtrofin wrote:
> > wenlei wrote:
> > > curious why do we need anonymous namespace here?
> > iiuc it's preferred we place file-local types inside an anonymous namespace. 
> > 
> > Looking now at the [[ https://llvm.org/docs/CodingStandards.html#anonymous-namespaces | style guideline ]], it touts their benefits but also says I should have only placed de decl there and the impl of those members out... but the members are quite trivial. Happy to move them out though.
> Thanks for the pointer. I don't have a strong opinion but slightly leaning towards moving out of anonymous namespace be consistent with how other InlineAdvice is organized (DefaultInlineAdvice, MLInlineAdvice not in anonymous namespace).
Ah, those are public (i.e. in a .h file)


================
Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:72-89
+  void recordUnsuccessfulInliningImpl(const InlineResult &Result) override {
+    if (IsInliningRecommended)
+      ORE.emit([&]() {
+        return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc, Block)
+               << "'" << NV("Callee", Callee) << "' is not AlwaysInline into '"
+               << NV("Caller", Caller)
+               << "': " << NV("Reason", Result.getFailureReason());
----------------
aeubanks wrote:
> can we add a test for these?
I think that would be tricky, because they should not actually happen - the way we determine whether a site is alwaysinlinable checks (but not thoroughly) for legality. Let me see if I can find a regression test. It may be we can synthesize such a case in IR only, though, so not much of a help for the frontend tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110891