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