Some minor update in PassManager.h

[dinesh.d]

Summary

This patch results in reduction of few PassModel classes. I have checked these
changes with existing test cases.

As per my understanding, runImpl will be statically resolved and inlined and will
not cause any function call overhead. In case, run methods changes to take more
arguments, though possibility is rare, this will be easy to extend.

Are these changes ok?

[dinesh.d]

forgot to CC to llvm-commits

[dinesh.d]

Resending with my comments [adding cc to Phab hasn't added my comments
when I added llvm-commits in cc]

This patch results in reduction of few PassModel classes. I have checked these
changes with existing test cases.

As per my understanding, runImpl will be statically resolved and inlined and will
not cause any function call overhead. In case, run methods changes to take more
arguments, though possibility is rare, this will be easy to extend.

Are these changes ok?

[dinesh.d]

gentle ping

[dinesh.d]

Gentle ping

[dinesh.d]

gentle ping

[chandlerc]

Sorry for not getting to this for so long. I really like the technique and I think there are a few other places wher ewe could apply it.

Can you add some doxygen comments for the utility? At some point I think we may want to pull it out of the pass manager and make it widely available in LLVM.

include/llvm/IR/PassManager.h

163 ↗

(On Diff #8922)

I really like this technique. I'd name it "invoke_with_optional_args" or something similar.

Also, you should use perfect forwarding here.

[dinesh.d]

wow, thanks for looking into this. I will do needful and update this patch.

Actaully, I stoped following new PassManager changes thinking that I may not be able to contribute due to my limited understanding about it.

I will update this patch, rebase it to latest codebase and submit it after testing ASAP.

[dinesh.d]

I have updated patch as per comments and re-based it with trunk. But I am facing
problem while using perfect forwarding as you have suggested. Type A1 and A2 is
deduced from passed function type. In our case, A2 is always pointer, just using
&&' in template parameter is not working.

Anyways, I have tried to use proper reference types so that wrapper will not cause
any temp object creation.

I will further read/ experiment and will try to make this code more generic.

[dinesh.d]

Sorry if it does not make any sense but following is my analysis on why we can't use
perfect forwarding here with the trick.

Conclusion:
We can use perfect forwarding for an argument with my trick only of given method (run)
takes that argument by reference.

Explanation:
Reference collapsing always return a reference, for a l-value or l-value reference to a
function template taking universal reference, it will treat it as l-value reference and for
r-value reference, as r-value reference. So type of invokeWithOptionalArgs's last 2
arguments, if we use perfect forwarding, will always be deduced as reference to some type.

In invokeWithOptionalArgs, we are trying to check if passed class (C) has given method
(f) or not, which deduce A1, A2 etc as parameter type of C:;f. If it does not match with
the deduced type for A1 and A2 from last 2 parameters (IR and AM) then that version
of invokeWithOptionalArgs will be removed from overload resolution (SFINAE).

In the case here, Run method takes AnalysisManager as pointer but if we use perfect
forwarding, A2 gets deduced as (dues to last argument passed to invokeWithOptionalArgs
(AM)) reference to AnalysisManager pointer (SFINAE case)

For A1, perfect forwarding works fine as all pass takes IR as reference.

[dinesh.d]

Update r238305 changes to fix compilation issue with MSVC.

[dinesh.d]

small correction in comment

include/llvm/IR/PassManagerInternal.h

 namespace llvm {
 
 template <typename IRUnitT> class AnalysisManager;
 class PreservedAnalyses;
 
 /// \brief Implementation details of the pass manager interfaces.
 namespace detail {
 
+/// \brief Helper template to run a specific class method with optional
+/// arguments.
+///
+/// This functor can be used as generic utility to invoke specific method on
+/// give object of any class from a set of classes fulfilling following
+/// prerequisite.
+/// 1. All classes in set defines at-least one such method taking all fixed
+/// arguments with optional other arguments.
+/// 2. In all such methods, if there exist any 2 methods with same fixed
+/// argument, they must have at least one optional argument.
+/// 3. Return type of such methods defined in the class must be same.
+/// 4. All such methods defined in the class can take any number of optional
+/// arguments.
+/// 5. For successful invocation of '()' operator, given class method should
+/// match the arguments passed (fixed/ optional) i.e. if method take an argument
+/// as (value/ l-value reference/ r-value reference/ pointer), provided argument
+/// should be of type (value/ l-value reference/ r-value reference/ pointer). of
+/// same type
+/// Example
+///  struct Arg1 {}; struct Arg2 {}; struct Arg3 {}; struct Arg4 {};
+///  struct A { void Print(Arg1 a1, Arg2 a2, Arg3 a3, Arg4 a4) {} }
+///  struct B { void Print(Arg1 a1, Arg2 a2) {} }
+///  int main {
+///    A a, B b, Arg1 a1, Arg2 a2, Arg3 a3, Arg4 a4;
+///    Invoke<void, Arg1, Arg2> I;
+///    I(a, A::Print, a1, a2, a3, a4); // will invoke a.print(a1, a2, a3, a4)
+///    I(b, B::Print, a1, a2, a3, a4); // will invoke b.print(a1, a2)
+///  }
+template <typename ReturnT, typename... FixedArgs> struct Invoker {
+  /// \brief Helper template to run Object::method *with* optional arguments.
+  template <typename Object, typename... OptionalArgs>
+  ReturnT operator()(Object &obj,
+                     ReturnT (Object::*method)(FixedArgs..., OptionalArgs...),
+                     FixedArgs... FAs, OptionalArgs... OAs) {
+    return (obj.*method)(std::forward<FixedArgs>(FAs)...,
+                         std::forward<OptionalArgs>(OAs)...);
+  }
+
+  /// \brief Helper template to run Object::method *without* an optional
+  /// arguments.
+  template <typename Object, typename... OptionalArgs>
+  ReturnT operator()(Object &obj, ReturnT (Object::*method)(FixedArgs... FFAs),
+                     FixedArgs... FAs, OptionalArgs... /*OAs*/) {
+    return (obj.*method)(std::forward<FixedArgs>(FAs)...);
+  }
+};
+
 /// \brief Template for the abstract base class used to dispatch
 /// polymorphically over pass objects.
-template <typename IRUnitT> struct PassConcept {
-  // Boiler plate necessary for the container of derived classes.
-  virtual ~PassConcept() {}
+template <typename ResultT, typename IRUnitT> struct PassConceptBase {
+  virtual ~PassConceptBase() {}
 
   /// \brief The polymorphic API which runs the pass over a given IR entity.
   ///
   /// Note that actual pass object can omit the analysis manager argument if
   /// desired. Also that the analysis manager may be null if there is no
   /// analysis manager in the pass pipeline.
-  virtual PreservedAnalyses run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) = 0;
+  virtual ResultT run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) = 0;
 
   /// \brief Polymorphic method to access the name of a pass.
   virtual StringRef name() = 0;
 };
 
-/// \brief SFINAE metafunction for computing whether \c PassT has a run method
-/// accepting an \c AnalysisManager<IRUnitT>.
-template <typename IRUnitT, typename PassT, typename ResultT>
-class PassRunAcceptsAnalysisManager {
-  typedef char SmallType;
-  struct BigType {
-    char a, b;
-  };
-
-  template <typename T, ResultT (T::*)(IRUnitT &, AnalysisManager<IRUnitT> *)>
-  struct Checker;
-
-  template <typename T> static SmallType f(Checker<T, &T::run> *);
-  template <typename T> static BigType f(...);
-
-public:
-  enum { Value = sizeof(f<PassT>(nullptr)) == sizeof(SmallType) };
-};
+/// \brief Abstract concept of an transform pass.
+///
+/// This concept is parameterized over the IR unit that it can run over and
+/// returns status of preserved analyses.
+template <typename IRUnitT>
+using PassConcept = PassConceptBase<PreservedAnalyses, IRUnitT>;
 
 /// \brief A template wrapper used to implement the polymorphic API.
 ///
 /// Can be instantiated for any object which provides a \c run method accepting
 /// an \c IRUnitT. It requires the pass to be a copyable object. When the
 /// \c run method also accepts an \c AnalysisManager<IRUnitT>*, we pass it
 /// along.
 template <typename IRUnitT, typename PassT,
-          typename PreservedAnalysesT = PreservedAnalyses,
-          bool AcceptsAnalysisManager = PassRunAcceptsAnalysisManager<
-              IRUnitT, PassT, PreservedAnalysesT>::Value>
-struct PassModel;
-
-/// \brief Specialization of \c PassModel for passes that accept an analyis
-/// manager.
-template <typename IRUnitT, typename PassT, typename PreservedAnalysesT>
-struct PassModel<IRUnitT, PassT, PreservedAnalysesT, true>
-    : PassConcept<IRUnitT> {
+          typename PreservedAnalysesT = PreservedAnalyses>
+struct PassModel : PassConceptBase<PreservedAnalyses, IRUnitT> {
   explicit PassModel(PassT Pass) : Pass(std::move(Pass)) {}
   // We have to explicitly define all the special member functions because MSVC
   // refuses to generate them.
   PassModel(const PassModel &Arg) : Pass(Arg.Pass) {}
   PassModel(PassModel &&Arg) : Pass(std::move(Arg.Pass)) {}
   friend void swap(PassModel &LHS, PassModel &RHS) {
     using std::swap;
     swap(LHS.Pass, RHS.Pass);
   }
   PassModel &operator=(PassModel RHS) {
     swap(*this, RHS);
     return *this;
   }
 
+  /// \brief The model delegates to the \c PassT::run method.
   PreservedAnalysesT run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) override {
-    return Pass.run(IR, AM);
+    return invokeRunMethod(Pass, &PassT::run, IR, AM);
   }
   StringRef name() override { return PassT::name(); }
-  PassT Pass;
-};
 
-/// \brief Specialization of \c PassModel for passes that accept an analyis
-/// manager.
-template <typename IRUnitT, typename PassT, typename PreservedAnalysesT>
-struct PassModel<IRUnitT, PassT, PreservedAnalysesT, false>
-    : PassConcept<IRUnitT> {
-  explicit PassModel(PassT Pass) : Pass(std::move(Pass)) {}
-  // We have to explicitly define all the special member functions because MSVC
-  // refuses to generate them.
-  PassModel(const PassModel &Arg) : Pass(Arg.Pass) {}
-  PassModel(PassModel &&Arg) : Pass(std::move(Arg.Pass)) {}
-  friend void swap(PassModel &LHS, PassModel &RHS) {
-    using std::swap;
-    swap(LHS.Pass, RHS.Pass);
-  }
-  PassModel &operator=(PassModel RHS) {
-    swap(*this, RHS);
-    return *this;
-  }
-
-  PreservedAnalysesT run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) override {
-    return Pass.run(IR);
-  }
-  StringRef name() override { return PassT::name(); }
   PassT Pass;
+  Invoker<PreservedAnalysesT, IRUnitT &> invokeRunMethod;
 };
 
 /// \brief Abstract concept of an analysis result.
 ///
 /// This concept is parameterized over the IR unit that this result pertains
 /// to.
 template <typename IRUnitT> struct AnalysisResultConcept {
   virtual ~AnalysisResultConcept() {}
struct AnalysisResultModel<IRUnitT, PassT, ResultT, PreservedAnalysesT, true>
 
   ResultT Result;
 };
 
 /// \brief Abstract concept of an analysis pass.
 ///
 /// This concept is parameterized over the IR unit that it can run over and
 /// produce an analysis result.
-template <typename IRUnitT> struct AnalysisPassConcept {
-  virtual ~AnalysisPassConcept() {}
-
-  /// \brief Method to run this analysis over a unit of IR.
-  /// \returns A unique_ptr to the analysis result object to be queried by
-  /// users.
-  virtual std::unique_ptr<AnalysisResultConcept<IRUnitT>>
-  run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) = 0;
-
-  /// \brief Polymorphic method to access the name of a pass.
-  virtual StringRef name() = 0;
-};
+template <typename IRUnitT>
+using AnalysisPassConcept =
+    PassConceptBase<std::unique_ptr<AnalysisResultConcept<IRUnitT>>, IRUnitT>;
 
 /// \brief Wrapper to model the analysis pass concept.
 ///
 /// Can wrap any type which implements a suitable \c run method. The method
 /// must accept the IRUnitT as an argument and produce an object which can be
 /// wrapped in a \c AnalysisResultModel.
-template <typename IRUnitT, typename PassT,
-          bool AcceptsAnalysisManager = PassRunAcceptsAnalysisManager<
-              IRUnitT, PassT, typename PassT::Result>::Value>
-struct AnalysisPassModel;
-
-/// \brief Specialization of \c AnalysisPassModel which passes an
-/// \c AnalysisManager to PassT's run method.
 template <typename IRUnitT, typename PassT>
-struct AnalysisPassModel<IRUnitT, PassT, true> : AnalysisPassConcept<IRUnitT> {
+struct AnalysisPassModel : AnalysisPassConcept<IRUnitT> {
   explicit AnalysisPassModel(PassT Pass) : Pass(std::move(Pass)) {}
   // We have to explicitly define all the special member functions because MSVC
   // refuses to generate them.
   AnalysisPassModel(const AnalysisPassModel &Arg) : Pass(Arg.Pass) {}
   AnalysisPassModel(AnalysisPassModel &&Arg) : Pass(std::move(Arg.Pass)) {}
   friend void swap(AnalysisPassModel &LHS, AnalysisPassModel &RHS) {
     using std::swap;
     swap(LHS.Pass, RHS.Pass);
   }
   AnalysisPassModel &operator=(AnalysisPassModel RHS) {
     swap(*this, RHS);
     return *this;
   }
 
   // FIXME: Replace PassT::Result with type traits when we use C++11.
-  typedef AnalysisResultModel<IRUnitT, PassT, typename PassT::Result>
-      ResultModelT;
+  using ResultT = typename PassT::Result;
+  using ResultModelT = AnalysisResultModel<IRUnitT, PassT, ResultT>;
 
   /// \brief The model delegates to the \c PassT::run method.
   ///
   /// The return is wrapped in an \c AnalysisResultModel.
   std::unique_ptr<AnalysisResultConcept<IRUnitT>>
   run(IRUnitT &IR, AnalysisManager<IRUnitT> *AM) override {
-    return make_unique<ResultModelT>(Pass.run(IR, AM));
-  }
-
-  /// \brief The model delegates to a static \c PassT::name method.
-  ///
-  /// The returned string ref must point to constant immutable data!
-  StringRef name() override { return PassT::name(); }
-
-  PassT Pass;
-};
-
-/// \brief Specialization of \c AnalysisPassModel which does not pass an
-/// \c AnalysisManager to PassT's run method.
-template <typename IRUnitT, typename PassT>
-struct AnalysisPassModel<IRUnitT, PassT, false> : AnalysisPassConcept<IRUnitT> {
-  explicit AnalysisPassModel(PassT Pass) : Pass(std::move(Pass)) {}
-  // We have to explicitly define all the special member functions because MSVC
-  // refuses to generate them.
-  AnalysisPassModel(const AnalysisPassModel &Arg) : Pass(Arg.Pass) {}
-  AnalysisPassModel(AnalysisPassModel &&Arg) : Pass(std::move(Arg.Pass)) {}
-  friend void swap(AnalysisPassModel &LHS, AnalysisPassModel &RHS) {
-    using std::swap;
-    swap(LHS.Pass, RHS.Pass);
-  }
-  AnalysisPassModel &operator=(AnalysisPassModel RHS) {
-    swap(*this, RHS);
-    return *this;
-  }
-
-  // FIXME: Replace PassT::Result with type traits when we use C++11.
-  typedef AnalysisResultModel<IRUnitT, PassT, typename PassT::Result>
-      ResultModelT;
-
-  /// \brief The model delegates to the \c PassT::run method.
-  ///
-  /// The return is wrapped in an \c AnalysisResultModel.
-  std::unique_ptr<AnalysisResultConcept<IRUnitT>>
-  run(IRUnitT &IR, AnalysisManager<IRUnitT> *) override {
-    return make_unique<ResultModelT>(Pass.run(IR));
+    return make_unique<ResultModelT>(
+        invokeRunMethod(Pass, &PassT::run, IR, AM));
   }
 
   /// \brief The model delegates to a static \c PassT::name method.
   ///
   /// The returned string ref must point to constant immutable data!
   StringRef name() override { return PassT::name(); }
 
   PassT Pass;
+  Invoker<ResultT, IRUnitT &> invokeRunMethod;
 };
 
 } // End namespace detail
 }
 
 #endif