This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Use a type for representing branch points in `RegionBranchOpInterface`
ClosedPublic

Authored by zero9178 on Aug 29 2023, 8:43 AM.

Details

Summary

The current implementation is not very ergonomic or descriptive: It uses std::optional<unsigned> where std::nullopt represents the parent op and unsigned is the region number.
This doesn't give us any useful methods specific to region control flow and makes the code fragile to changes due to now taking the region number into account.

This patch introduces a new type called RegionBranchPoint, replacing all uses of std::optional<unsigned> in the interface. It can be implicitly constructed from a region or a RegionSuccessor, can be compared with a region to check whether the branch point is branching from the parent, adds isParent to check whether we are coming from a parent op and adds RegionSuccessor::parent as a descriptive way to indicate branching from the parent.

Diff Detail

Event Timeline

zero9178 created this revision.Aug 29 2023, 8:43 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Aug 29 2023, 8:43 AM

Glad someone finally went and did this :P

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
200

FWIW I think it's more idiomatic to have RegionBranchPointer::parent() instead of accessing the singleton directly. It mirrors WalkResult::advance(), LogicalResult::success(), etc.

205

Tagging /*implicit*/ is not usual for the MLIR coding style, or is it? I don't see it very often.

zero9178 updated this revision to Diff 554408.Aug 29 2023, 10:03 AM

Address review comments

zero9178 marked 2 inline comments as done.Aug 29 2023, 10:03 AM
zero9178 added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
205

Seems so! A quick grep and wc showed only 18 instances. I must have gotten the wrong impression

Mogball accepted this revision.Aug 29 2023, 10:23 AM
This revision is now accepted and ready to land.Aug 29 2023, 10:23 AM
This revision was landed with ongoing or failed builds.Aug 29 2023, 11:05 AM
This revision was automatically updated to reflect the committed changes.
zero9178 marked an inline comment as done.