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

Reply via email to