[PATCH] D58399: [Inliner] Don't initialize ComputeFullInlineCost to be always true because of ORE

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM (with a minor cleanup, but feel free to just submit w/ that if we're all agreeing about this direction).



================
Comment at: lib/Analysis/InlineCost.cpp:2008-2012
+  OptimizationRemarkEmitter NewORE(Callee);
+  if (!ORE &&
+      Callee->getContext().getDiagHandlerPtr()->isMissedOptRemarkEnabled(
+          DEBUG_TYPE))
+    ORE = &NewORE;
----------------
wmi wrote:
> chandlerc wrote:
> > wmi wrote:
> > > wmi wrote:
> > > > chandlerc wrote:
> > > > > you can just set `ORE` to `nullptr` here?
> > > > I don't get the idea. Could you explain it? Setting ORE to nullptr here then it won't compute the full inline cost even if -Rpass=inline* is used.
> > > Easwaran discussed with me offline. I think he has the same suggestion with you -- pass RemarksEnabled as another parameter of getInlineCost and set ORE here, so as to avoid creating another Emitter inside of getInlineCost. I will update the patch accordingly.
> > I do like this better, and my suggestion wasn't very clear as I misunderstood your first response. Sorry about that.
> > 
> > I wonder if we should go one step further: I think the idea that we look at both `DEBUG_TYPE`s is actually a bad thing. I'd much rather we *only* look at the pass's `DEBUG_TYPE` (personally). And it has the side-effect of simplifying this somewhat confusing structure here. Thoughts?
> That makes sense to me. Easwaran also suggested eventually optimization remarks emitted in InlineCost.cpp is better to be moved outside too -- return the related information to the caller and let the passes decide whether to emit the remarks. But that deserves a separate change. For now, I just simply remove the remark enabling check for inline-cost DEBUG_TYPE.
nit: You can probably just have the callers pass in a nullptr similar to your previous patch if they're the only ones doing this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58399