This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Introduce generic visitors.
ClosedPublic

Authored by jurahul on Dec 23 2021, 11:48 AM.

Details

Summary
  • Generic visitors invoke operation callbacks before/in-between/after visiting the regions attached to an operation and use a WalkStage to indicate which regions have been visited.
  • This can be useful for cases where we need to visit the operation in between visiting the regions attached to the operation. END_PUBLIC

Diff Detail

Event Timeline

jurahul created this revision.Dec 23 2021, 11:48 AM
jurahul requested review of this revision.Dec 23 2021, 11:48 AM
rriddle requested changes to this revision.Dec 23 2021, 1:51 PM
rriddle added inline comments.
mlir/include/mlir/IR/Operation.h
526

Missing the walk stage parameter here it seems.

mlir/include/mlir/IR/Visitors.h
77

Drop the mlir::

79–84

Functions should be camelCase.

87–88

Variables should be camelCase.

101–114

Use llvm::function_traits instead.

265

Drop the mlir on all of these.

267

Drop the llvm::

284

Drop the llvm::

mlir/lib/IR/Visitors.cpp
77–83

nit: Drop the auto on these,

This revision now requires changes to proceed.Dec 23 2021, 1:51 PM
jurahul updated this revision to Diff 396168.Dec 24 2021, 10:30 AM

Address review comments.

jurahul marked 8 inline comments as done.Dec 24 2021, 10:35 AM
rriddle added inline comments.Jan 9 2022, 11:03 PM
mlir/include/mlir/IR/Visitors.h
79–84

Can you add a short doc comment to each of these?

242–255

Can you add support for the WalkOrder for these? To match parity with the other callbacks.

jurahul updated this revision to Diff 399509.Jan 12 2022, 4:36 PM

Address review comments

jurahul marked 2 inline comments as done.Jan 12 2022, 4:38 PM
jurahul added inline comments.
mlir/include/mlir/IR/Visitors.h
242–255

I'd think the WalkOrder parameter does not make sense for generic visitors since the visit is a combination of pre/in/post order.

Thanks, Rahul for contributing this! It's great that it can be integreated with the existing walk methods. LGTM. I'll leave the final approval to @rriddle.

mlir/include/mlir/IR/Visitors.h
79

Return if -> Return true? Maybe I'm not parsing this correctly. Same below.

jurahul added inline comments.Jan 13 2022, 10:00 AM
mlir/include/mlir/IR/Visitors.h
79

Yeah, some places seem to use "Returns if the current range is empty." and others "Returns true if the operation has been registered, i.e. if the". I agree that "Returns true if" is easier to read, will update.

rriddle accepted this revision.Jan 13 2022, 10:32 AM

LGTM after resolving comments.

mlir/include/mlir/IR/Visitors.h
270–274

All of these should use /// for comments.

290–299
mlir/test/lib/IR/TestVisitorsGeneric.cpp
24–25

Top level comments should use ///

118–124

A common thing is to have one exposed function that registers multiple test passes.

This revision is now accepted and ready to land.Jan 13 2022, 10:32 AM
jurahul updated this revision to Diff 399733.Jan 13 2022, 11:08 AM

Address review comments

jurahul updated this revision to Diff 399769.Jan 13 2022, 1:32 PM

Address River's comments.

jurahul marked 5 inline comments as done.Jan 13 2022, 3:37 PM
This revision was automatically updated to reflect the committed changes.
bondhugula added inline comments.
mlir/include/mlir/IR/Visitors.h
95–96

unsigned?
Missing doc comments here.

292–293

The second sentence "... is selected for WalkReturn returning interruptible callbacks ..." isn't clear. What's WalkReturn??