-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement support for pre-visitor callbacks. #84
base: dev
Are you sure you want to change the base?
Conversation
c499e94
to
73945aa
Compare
This change implements a mechanism to run pre-visit callbacks on each operation it stumbles upon. This can be generalized for every op that is being visited, for the current nesting level, or for a given `OpSet`. The code simplifies writing visitors in cases where generic code is written for a set operations, like resetting the insertion point of an `IRBuilder`.
73945aa
to
d7f2314
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about this API. I think I'd prefer something that is more clearly structured. Something along the lines of:
static const auto visitor = ...
.add(&Blah::visitFoo)
.withPreVisit(
&Blah:somePreVisit, // somePreVisit(Instruction &inst)
[](VisitorBuilder<Blah> &b) {
b.add(&Blah::visitBar);
b.add(&Blah::visitBaz);
})
...
There's also a question of what, if anything, the pre-visitor should return. I think it could be one of:
- Continue -- go to the visitor that is "guarded" by the pre-visitor
- Stop -- stop visiting entirely
- SKip -- skip the "guarded" visitor(s) but potentially continue to other matching visitors
@@ -91,7 +91,7 @@ class OpSet final { | |||
// arguments. | |||
template <typename... OpTs> static const OpSet get() { | |||
static OpSet set; | |||
(... && appendT<OpTs>(set)); | |||
(void)(... && appendT<OpTs>(set)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine I suppose, but see #87
Or perhaps it shouldn't be a "pre-visitor" but a sort of scope guard: .withGuard(&Blah::someGuard, [](VisitorBuilder<Blah> &b) { ... })
...
VisitorResult Blah::someGuard(Instruction &inst, llvm::function_ref<VisitorResult()> visit) {
if (should skip)
return VisitorResult::Continue;
do some setup
auto result = visit(); // this calls the visitors that were registered inside of the "withGuard" block
do some teardown
return result;
} |
Having separate setup and teardown callbacks per-op (or for all ops) as part of the guards sounds like a good way, and we'd nest all visitor callbacks in a top-level guard with a setup callback to do something like setting the Builder insertion point. |
This change implements a mechanism to run pre-visit callbacks on each operation it stumbles upon. This can be generalized for every op that is being visited, for the current nesting level, or for a given
OpSet
.The code simplifies writing visitors in cases where generic code is written for a set operations, like resetting the insertion point of an
IRBuilder
.Note: the support is currently basic. When adding a pre-visitor callback for a given nesting level without an operation, this will operate for each instruction types with visitor callbacks. I'd like to extend this so we only operate on the set of operations registered for the current visitor nesting level or its parents, but I'm afraid we don't support that yet.
In addition, I plan on cleaning up all of the forwarding code and surroundings next, since it became a bit of a mess with all of the repetetive code.
@nhaehnle