Skip to content
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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

tsymalla
Copy link
Contributor

@tsymalla tsymalla commented Feb 1, 2024

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

@tsymalla tsymalla force-pushed the implement_previsitcallback branch from c499e94 to 73945aa Compare February 2, 2024 07:41
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`.
@tsymalla tsymalla force-pushed the implement_previsitcallback branch from 73945aa to d7f2314 Compare February 2, 2024 10:38
Copy link
Member

@nhaehnle nhaehnle left a 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));
Copy link
Member

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

@nhaehnle
Copy link
Member

nhaehnle commented Feb 2, 2024

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;
}

@tsymalla-AMD
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants