[PATCH] D47858: [New PM] Introducing PassInstrumentation framework
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 15 03:09:43 PDT 2018
chandlerc added a comment.
It gets better and better. Another round of comments.
================
Comment at: include/llvm/IR/PassInstrumentation.h:16
+/// - PassInstrumentation provides a set of instrumentation points for
+/// pass managers to call on. It is light on copy since it is queries
+/// through the analysis framework as a result of the analysis.
----------------
"IT is light on copy since it is queries" -> "It is cheap to copy since it queries"
================
Comment at: include/llvm/IR/PassInstrumentation.h:17
+/// pass managers to call on. It is light on copy since it is queries
+/// through the analysis framework as a result of the analysis.
+///
----------------
Not sure this explanation of why the copy is cheap quite parses for me.
================
Comment at: include/llvm/IR/PassInstrumentation.h:20
+/// - PassInstrumentationCallbacks registers callbacks and provides access
+/// - to them for PassInstrumentation.
+///
----------------
extraneous `-`.
================
Comment at: include/llvm/IR/PassInstrumentation.h:107-113
+ void registerBeforePassCallback(const std::function<BeforePassFunc> &C) {
+ BeforePassCallbacks.push_back(C);
+ }
+
+ void registerAfterPassCallback(const std::function<AfterPassFunc> &C) {
+ AfterPassCallbacks.push_back(C);
+ }
----------------
I think this is one of those weird places and times where `emplace` and such is what you want.
Specifically, I think you should do:
```
template <typename CallableT> void registerAfterPassCallback(CallableT C) {
AfterPassCallback.emplace_back(std::move(C));
}
```
Or something of the sort. You want to support passing a lambda w/ move only captures and not allocating memory for it in the caller, but only inside the vector.
================
Comment at: include/llvm/IR/PassInstrumentation.h:120-131
+ /// BeforePass instrumentation point - takes \p PassID identifier for the
+ /// pass instance to be executed and constant reference to IR it operates on.
+ /// Note, that PassID currently is not meant to be unique per-pass-instance.
+ /// \Returns true if pass is allowed to be executed.
+ template <typename IRUnitT>
+ bool runBeforePass(StringRef PassID, const IRUnitT &) const;
+
----------------
Since you already befriend this class, why not just inline thes functions into their callers below?
================
Comment at: include/llvm/IR/PassInstrumentation.h:133
+
+ // instrumentation callbacks
+ SmallVector<std::function<BeforePassFunc>, 4> BeforePassCallbacks;
----------------
I don't think this comment is really necessary.
================
Comment at: include/llvm/IR/PassInstrumentation.h:134-135
+ // instrumentation callbacks
+ SmallVector<std::function<BeforePassFunc>, 4> BeforePassCallbacks;
+ SmallVector<std::function<AfterPassFunc>, 4> AfterPassCallbacks;
+};
----------------
I would suggest using `llvm::unique_function` throughout this code to support callbacks with move-only captures.
I generally suspect we should default to `llvm::unique_function` over `std::function` at this point until a need for copy arises.
================
Comment at: include/llvm/IR/PassInstrumentation.h:144-145
+public:
+ PassInstrumentation(const PassInstrumentationCallbacks *CB = nullptr)
+ : Callbacks(CB) {}
+
----------------
Comment on ownership and lifetime?
================
Comment at: include/llvm/IR/PassInstrumentation.h:70
+/// that need to track pass execution.
+class PassExecutionCounter {
+ unsigned count = 0;
----------------
fedor.sergeev wrote:
> philip.pfaffe wrote:
> > Is this still required?
> if I got Chandler right he asked me to keep it around for individual callbacks use.
> I'm really not sure that it is needed anymore.
Sorry for ambiguity. I think you can pull it out for now. I would re-introduce it as part of the bisection in a way that fits with that framework.
================
Comment at: include/llvm/IR/PassManager.h:484-490
+ // Request PassInstrumentation from analysis manager, will use it to run
+ // instrumenting callbacks for the passes later.
+ // Here we use std::tuple wrapper over getResult which helps to extract
+ // AnalysisManager's arguments out of the whole ExtraArgs set.
+ PassInstrumentation PI =
+ detail::getAnalysisResult<PassInstrumentationAnalysis>(
+ AM, IR, std::tuple<ExtraArgTs...>(ExtraArgs...));
----------------
Rather than assume we can do this generically with tuple-hacking of the parameter pack, I'd suggest simply assuming that the extra arguments match, and insisting for cases where they do not match we use explicit specializations of the relevant `run` method there to manually implement the logic.
We already do some of this. If it truly becomes problematic, I think we could solve it a different way instead, but I'm not sure it is worthwhile without seeing how many (and how much logic) ends up being duplicated w/ the specializations.
The reason I prefer specializations is that it places no firm contract on the relationship between the arguments. I'd prefer to not bake in such a relationship to the API because then we may end up designing things in awkward ways to satisfy this constraint.
================
Comment at: include/llvm/IR/PassManager.h:568-571
+/// Pseudo-analysis pass that exposes the \c PassInstrumentation to pass
+/// managers. Goes before AnalysisManager definition to provide its
+/// internals (e.g PassInstrumentationAnalysis::ID) for use there if needed.
+class PassInstrumentationAnalysis
----------------
I think we should either move all the instrumentation stuff into this header, or find a way to pull this into the separate header. I think the latter is possible but will require some refactorings. Can you leave a FIXME for now so that we come back to this?
================
Comment at: include/llvm/IR/PassManager.h:578
+ /// analysis.
+ PassInstrumentationCallbacks *PIC;
+
----------------
I think a name like `Callbacks` would be easier to read than `PIC`.
================
Comment at: include/llvm/IR/PassManager.h:762-764
+ template <typename PassT> bool isRegisteredAnalysis() const {
+ return AnalysisPasses.count(PassT::ID()) != 0;
+ }
----------------
Is this still needed?
================
Comment at: include/llvm/IR/PassManagerInternal.h:51
/// Polymorphic method to access the name of a pass.
- virtual StringRef name() = 0;
+ virtual StringRef name() const = 0;
};
----------------
This and all of the other changes to `PassManagerInternal.h` are really orthogonal cleanups and LGTM. Please pull them into their own patch and go ahead and land that.
================
Comment at: include/llvm/Passes/PassBuilder.h:63-65
+ PassInstrumentationCallbacks *getPassInstrumentationCallbacks() const {
+ return PIC;
+ }
----------------
Since the code constructing this has access to the callbacks already, why do we need to expose it? I'd rather keep the API narrow (like we do for `TM` above).
================
Comment at: include/llvm/Passes/PassBuilder.h:564-568
+ /// Calls registration function \p C on PassInstrumentationCallbacks object
+ /// to register pass instrumentation callbacks there.
+ ///
+ /// \Returns false if there is no pass instrumentation object available.
+ bool registerPassInstrumentation(
----------------
I'm somewhat curious what the expected usage of this method is? It isn't obvious to me at all...
================
Comment at: lib/Passes/PassBuilder.cpp:318-320
+ MAM.registerPass([&] {
+ return PassInstrumentationAnalysis(this->getPassInstrumentationCallbacks());
+ });
----------------
No need for any of this -- you can use the `PassRegistrsy.def` and write `PassInstrumentationAnalysis(PIC)`. That's how we already do things with `TM`.
================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:268-272
+ // Generic EXPECT_CALLs are needed to match instrumentation on unimportant
+ // parts of a pipeline that we do not care about (e.g. various passes added
+ // by default by PassBuilder - Verifier pass etc).
+ EXPECT_CALL(*this, runBeforePass(_, _)).Times(AnyNumber());
+ EXPECT_CALL(*this, runAfterPass(_, _)).Times(AnyNumber());
----------------
Instead of this, you can use `::testing::NiceMock<MockPassInstrumentationCallbacks>` in places where you want to ignore these. The way you've written it, even if users *want* to check for unexpected calls to these routines, they won't be able to do so.
================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:283-284
+/// Helper for HasName matcher that returns getName both for IRUnit and
+/// for IRUnit pointer wrapper into llvm::Any (wrapped by PassInstrumentation).
+template <typename IRUnitT> StringRef getName(const IRUnitT &IR) {
----------------
Maybe leave a FIXME here?
We should design a good matcher for `llvm::Any` and then we should be able to compose things. But this is fine for now.
================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:301
+ return "<UNKNOWN>";
+}
+/// Define a custom matcher for objects which support a 'getName' method.
----------------
Missing vertical space.
================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:423-424
+ PIC.registerBeforePassCallback(
+ std::bind(&MockPassInstrumentationCallbacks::runBeforePass,
+ &PICallbacks, _1, _2));
+ PIC.registerAfterPassCallback(
----------------
`std::bind` should generally be avoided at this point. We can just use lambdas to delegate here.
And see my comment on `registerPassInstrumentian` above: I think instead we should just register the callbacks first, and hand the object into the pass builder.
Notably, I think you can have the mock *contain* a `PassInstrumentationCallbacks` object that it pre-registers with lambdas that delegate to the mock, and then just pass that into `PassBuilder` constructor.
================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:463-464
+
+ {
+ testing::InSequence InstrumentationExpectationsSequenced;
+ EXPECT_CALL(PICallbacks, runBeforePass(HasNameRegex("MockPassHandle"),
----------------
I find it simpler to define a `Sequence` object and then connect it to each expectation with the `.InSequence` methods. This makes it fairly easy to avoid a proliferation of scopes to wire these things up.
You'll also want to say how *many* calls here, which will make more sense once you make the change I suggest above w.r.t. the number.
Repository:
rL LLVM
https://reviews.llvm.org/D47858
More information about the llvm-commits
mailing list