This is an archive of the discontinued LLVM Phabricator instance.

[mlir][IR] Add Iterator template option to IR walkers
ClosedPublic

Authored by springerm on Feb 17 2023, 3:57 AM.

Details

Summary

This allows users to specify a top-down or bottom-up traversal of the IR, in addition to the already existing WalkOrder.

Certain transformations work better with a forward traversal. E.g., when cloning a piece of IR, operations should be cloned top-down so that all uses are defined when creating an op.

Certain transformations work better with a reverse traversal. E.g., when erasing a piece of IR, operations should be erased bottom-up to avoid erasing operations that still have users.

Diff Detail

Event Timeline

springerm created this revision.Feb 17 2023, 3:57 AM
springerm requested review of this revision.Feb 17 2023, 3:57 AM
springerm added inline comments.Feb 17 2023, 4:40 AM
mlir/lib/IR/Visitors.cpp
42–50

Note: Explicit instantiations would not be needed if detail::walk would be defined in the header file.

springerm retitled this revision from [mlir][IR] Add IteratorOrder option to IR walkers to [mlir][IR] Add Iterator template option to IR walkers.Feb 17 2023, 5:22 AM
springerm edited the summary of this revision. (Show Details)

This change overall makes sense to me

mlir/lib/IR/Visitors.cpp
31

Can you implement this with if constexpr instead? Walking is very performance sensitive and it's better not to regress existing code.

springerm updated this revision to Diff 499475.Feb 22 2023, 6:35 AM

address comments

springerm marked an inline comment as done.Feb 22 2023, 6:38 AM
springerm added inline comments.
mlir/lib/IR/Visitors.cpp
31

I was able to simplify TopDownIterator::makeRange and make it a constexpr function that simplify passes through the argument as a reference. Together with Iterator being a template argument, there should be no difference in performance in case of a top-down traversal.

springerm marked an inline comment as done.Feb 22 2023, 6:43 AM
rriddle added inline comments.Feb 22 2023, 9:29 AM
mlir/include/mlir/IR/Visitors.h
66–83

Why not just name them "ForwardIterator" and "ReverseIterator" instead of TopDown/BottomUp (which aren't correct, given the hierarchal structure of the IR).

address comments

springerm marked an inline comment as done.Feb 23 2023, 12:12 AM
Mogball accepted this revision.Feb 23 2023, 3:14 PM
This revision is now accepted and ready to land.Feb 23 2023, 3:14 PM
This revision was landed with ongoing or failed builds.Feb 24 2023, 1:24 AM
This revision was automatically updated to reflect the committed changes.