[PATCH] D58406: Fix IR/Analysis layering issue in OptBisect

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/IR/OptBisect.h:29
 
-  virtual bool shouldRunPass(const Pass *P, const Module &U) { return true; }
-  virtual bool shouldRunPass(const Pass *P, const Function &U)  {return true; }
-  virtual bool shouldRunPass(const Pass *P, const BasicBlock &U)  { return true; }
-  virtual bool shouldRunPass(const Pass *P, const Region &U)  { return true; }
-  virtual bool shouldRunPass(const Pass *P, const Loop &U)  { return true; }
-  virtual bool shouldRunPass(const Pass *P, const CallGraphSCC &U)  { return true; }
+  virtual bool shouldRunPass(const Pass *P, StringRef IRDescription) {
+    return true;
----------------
philip.pfaffe wrote:
> fedor.sergeev wrote:
> > So, since we went over changing the interface to use StringRef for description, can we move one more step further
> > in that direction and start passing StringRef for Pass?
> > That would make this interface fully ready for new pass manager.
> > 
> > I can do it later on myself, but doing it here would lessen the churn by doing one change in interface, not two of them.
> > 
> > (it will not add any more overhead since pass name is always available as a string literal)
> I concur with @andrew.w.kaylor that the performance hit from building the IRDescription will probably be fine. Nevertheless, it should be much cheaper to pass a Twine here instead! Additionally there should be a comment here explaining IRDescription.
I'd probably still suggest doing it as two changes - independent changes in independent commits and all that. But I don't feel especially strongly about it/this isn't code I have much ownership of/investment in.

As for Twine - not sure that would be relevant here. The most expensive ones don't benefit from Twine anyway (only those where "getDescription" could be removed, and the implementation written inline in the call to shouldRunPass could be Twine-able) and now that the condition where the string isn't needed is hoisted out before the call - once we're at the point of producing the description we know we're going to need it in a string, so there's nothing to be gained by delaying through a Twine, I think?


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

https://reviews.llvm.org/D58406