This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Revamp `RegionBranchOpInterface` successor mechanism
ClosedPublic

Authored by zero9178 on Aug 9 2023, 7:27 AM.

Details

Summary

The RegionBranchOpInterface had a few fundamental issues caused by the API design of getSuccessorRegions.

It always required passing values for the operands parameter. This is problematic as the operands parameter actually changes meaning depending on which predecessor index is referring to. If coming from a region, you'd have to find a RegionBranchTerminatorOpInterface in that region, get its operand count, and then create a SmallVector of that size.
This is not only inconvenient, but also error-prone, which has lead to a bug in the implementation of a previously existing getSuccessorRegions overload.

Additionally, this made the method dual-use, trying to serve two different use-cases: 1) Trying to determine possible control flow edges between regions and 2) Trying to determine the region being branched to based on constant operands.

This patch fixes these issues by changing the interface methods and adding new ones:

  • The operands argument of getSuccessorRegions has been removed. The method is now only responsible for returning possible control flow edges between regions.
  • An optional getEntrySuccessorRegions method has been added. This is used to determine which regions are branched to from the parent op based on constant operands of the parent op. By default, it calls getSuccessorRegions. This is analogous to getSuccessorForOperands from BranchOpInterface.
  • Add getSuccessorRegions to RegionBranchTerminatorOpInterface. This is used to get the possible successors of the terminator based on constant operands. By default, it calls the containing RegionBranchOpInterfaces getSuccessorRegions method.
  • getSuccessorEntryOperands was renamed to getEntrySuccessorOperands for consistency.

Diff Detail

Event Timeline

zero9178 created this revision.Aug 9 2023, 7:27 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
zero9178 requested review of this revision.Aug 9 2023, 7:27 AM
mehdi_amini added inline comments.Aug 9 2023, 10:15 PM
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
256–264

Technical nit: a region terminator never directly "branch to" another region, the parent operation is driving the control flow.

267

Why do we pass this array instead of letting the implementation match as needed?

zero9178 updated this revision to Diff 548888.Aug 9 2023, 11:53 PM
zero9178 marked an inline comment as done.

Address review comments

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
256–264

I've changed from to after in the wording, with the intention that it does not imply the branching is done by the terminator.

267

The reason is very similar to why fold does it: This makes thes interface more useful as you can pass in constant values and "see what it does".
This is used by various DataFlowAnalysis passes for example. SCCP will pass in constants if it has so far optimistcally proven that a given value has only been a constant so far.
Combined with the region branch op interface, it enables more dead code analysis.

If you'd want to do this using matchPattern, a caller would have to first materialize n constant ops, change the operands of the op to the constant ops, call getSuccessorRegions, and then undo the transformation (as it was performed optimistically on purpose and could still end up being incorrect).

I'll let @Mogball maybe review with more details, but thanks for the fixes!

Mogball accepted this revision.Aug 10 2023, 1:02 AM
Mogball added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
180–181

Perfect. This API was definitely not ideal. The new method makes things abundantly clear.

271

style: prefer using the original namespace if explicitly spelled out

294
This revision is now accepted and ready to land.Aug 10 2023, 1:02 AM
zero9178 marked 3 inline comments as done.Aug 10 2023, 1:35 AM
This revision was landed with ongoing or failed builds.Aug 10 2023, 1:35 AM
This revision was automatically updated to reflect the committed changes.

Sadly this breaks my local build as well as one of the bots:

Most likely missing CMake dependency, but it's a bit too large for me to identify it myself. I will revert it, but please let me know if you are unable to reproduce locally and need any help with this.

Sadly this breaks my local build as well as one of the bots:

Most likely missing CMake dependency, but it's a bit too large for me to identify it myself. I will revert it, but please let me know if you are unable to reproduce locally and need any help with this.

Already fixed in https://github.com/llvm/llvm-project/commit/345acac85d382998e5721073cc077e47e799311f. Sorry for the inconvenience

Sadly this breaks my local build as well as one of the bots:

Most likely missing CMake dependency, but it's a bit too large for me to identify it myself. I will revert it, but please let me know if you are unable to reproduce locally and need any help with this.

Already fixed in https://github.com/llvm/llvm-project/commit/345acac85d382998e5721073cc077e47e799311f. Sorry for the inconvenience

No bother at all, thanks for the quick fix!