Page MenuHomePhabricator

Added new RegionBranchTerminatorOpInterface and adapted uses of hasTrait<ReturnLike>.
ClosedPublic

Authored by dfki-mako on Jun 28 2021, 5:54 AM.

Details

Summary

This CL adds a new RegionBranchTerminatorOpInterface to query information about operands that can be
passed to successor regions. Similar to the BranchOpInterface, it allows to freely define the
involved operands. However, in contrast to the BranchOpInterface, it expects an additional region
number to distinguish between various use cases which might require different operands passed to
different regions.

Moreover, we added new utility functions (namely getMutableRegionBranchSuccessorOperands and
getRegionBranchSuccessorOperands) to query (mutable) operand ranges for operations equiped with the
ReturnLike trait and/or implementing the newly added interface. This simplifies reasoning about
terminators in the scope of the nested regions.

We also adjusted the SCF.ConditionOp to benefit from the newly added capabilities.

See also: discussion on Discourse

Diff Detail

Event Timeline

dfki-mako created this revision.Jun 28 2021, 5:54 AM
dfki-mako requested review of this revision.Jun 28 2021, 5:54 AM
dfki-mako edited the summary of this revision. (Show Details)Jun 28 2021, 5:56 AM
dfki-mako added reviewers: rriddle, herhut, mehdi_amini.
dfki-mako edited the summary of this revision. (Show Details)
rriddle requested changes to this revision.Jun 28 2021, 11:57 AM

Thanks, this seems like a natural evolution.

mlir/include/mlir/Dialect/SCF/SCFOps.td
38–45
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
193–194

Can you fill this in?

201

Can you reference the specific parameters of the method here? So that it is clear what the represent?

mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
105–109
115

You'll also need to update many other usages of the control flow interfaces to use this. Have you considered adding a utility/<something-else> for this? So that users don't need to check multiple interfaces/traits? e.g. if ReturnLike in ODS also implied this interface somehow.

121

If one side of the branch has braces, both sides should.

This revision now requires changes to proceed.Jun 28 2021, 11:57 AM
mehdi_amini added inline comments.Jun 28 2021, 5:53 PM
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
121

Alternatively, add continue to the then branch and eliminate the else and reduce indentation afterward.

dfki-mako updated this revision to Diff 358186.Jul 13 2021, 1:09 AM
dfki-mako marked 7 inline comments as done.

Addressed reviewer comments and refined RegionBranchTerminatorOpInterface.
Adapted uses of hasTrait<ReturnLike> to use newly added functions (if possible).

dfki-mako retitled this revision from RFC: Added new RegionBranchTerminatorOpInterface and provided a sample implementation for the SCF.ConditionOp. to Added new RegionBranchTerminatorOpInterface and adapted uses of hasTrait<ReturnLike>..Jul 13 2021, 1:10 AM
dfki-mako edited the summary of this revision. (Show Details)

@rriddle there is also another use of the ReturnLike trait in the scope of the DataFlowAnalysis. Currently, you check whether a terminator is ReturnLike. If not, you mark the following region successors to have reached a pessimistic fixpoint. This raises the question of whether we want to change this behavior to propagate lattice information in the context of "more return-like" operations.

The following patch would change this behavior to include all operations that are either marked as ReturnLike or implement the RegionBranchTerminatorOpInterface:

@@ -562,26 +562,28 @@ void ForwardDataFlowSolver::visitTerminatorOperation(
     if (regionSuccessors.empty())
       return;
 
+    // Try to get "region-like" successor operands if possible in order to
+    // propagate the operand states to the successors.
+    if (isRegionReturnLike(op))
+      return visitRegionSuccessors(
+          parentOp, regionSuccessors, [&](Optional<unsigned> regionIndex) {
+            // Determine the individual region successor operands for the given
+            // region index (if any).
+            return *getRegionBranchSuccessorOperands(op, regionIndex);
+          });
+

However, I am completely fine with *not* changing this code to make sure that lattice states are propagated to region successors in the scope of "real return-like" operations only.

@rriddle there is also another use of the ReturnLike trait in the scope of the DataFlowAnalysis. Currently, you check whether a terminator is ReturnLike. If not, you mark the following region successors to have reached a pessimistic fixpoint. This raises the question of whether we want to change this behavior to propagate lattice information in the context of "more return-like" operations.

The following patch would change this behavior to include all operations that are either marked as ReturnLike or implement the RegionBranchTerminatorOpInterface:

@@ -562,26 +562,28 @@ void ForwardDataFlowSolver::visitTerminatorOperation(
     if (regionSuccessors.empty())
       return;
 
+    // Try to get "region-like" successor operands if possible in order to
+    // propagate the operand states to the successors.
+    if (isRegionReturnLike(op))
+      return visitRegionSuccessors(
+          parentOp, regionSuccessors, [&](Optional<unsigned> regionIndex) {
+            // Determine the individual region successor operands for the given
+            // region index (if any).
+            return *getRegionBranchSuccessorOperands(op, regionIndex);
+          });
+

However, I am completely fine with *not* changing this code to make sure that lattice states are propagated to region successors in the scope of "real return-like" operations only.

I would include RegionBranchTerminatorOpInterface handling in that code as well. Using RegionBranchTerminatorOpInterface should allow for providing a more refined result in cases that use it.

dfki-mako updated this revision to Diff 358576.Jul 14 2021, 6:13 AM

Adatped ForwardDataFlowSolver to support the newly added RegionBranchTerminatorOpInterface.

herhut accepted this revision.Jul 19 2021, 3:25 AM

What is the longer term plan to get rid of ReturnLike? This can land in the meantime as a first step in the right direction, though.

rriddle requested changes to this revision.Jul 19 2021, 3:26 AM

What is the longer term plan to get rid of ReturnLike? This can land in the meantime as a first step in the right direction, though.

I'd prefer if you can wait until I have time to look it over. I'm currently OOO for the next few days.

This revision now requires changes to proceed.Jul 19 2021, 3:26 AM
zero9178 added inline comments.
mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
207–217

Could the types here be fully qualified (eg. ::mlir::OperandRange instead of OperandRange)? Otherwise it is impossible to use this interface in a downstream project without also putting using namespace mlir; into the header.

@herhut I think it might be beneficial to either remove the ReturnLike trait or to change it in a way such that it automatically provides a "default implementation" of the newly added RegionBranchTerminatorOpInterface.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
207–217

Hmm, yes we can do that :) However, there are more occurrences in the same file without explicitly qualified OperandRange/MutableOperandRange types.
@rriddle Should we also explicitly qualify all other uses?

@herhut I think it might be beneficial to either remove the ReturnLike trait or to change it in a way such that it automatically provides a "default implementation" of the newly added RegionBranchTerminatorOpInterface.

I think ReturnLike is common enough to have it as a default implementation. Otherwise we would have to implement this again and again. I see it similar to FunctionLike.

Added benefit, it provides a transition path.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
207–217

The other cases likely are an oversight and would have the same effect. So if you fix them, too, that would be great.

dfki-mako updated this revision to Diff 360746.Jul 22 2021, 2:48 AM
dfki-mako marked 2 inline comments as done.

Changed namespace qualifiers to explicitly reference the llvm/mlir namespaces.

rriddle accepted this revision.Jul 22 2021, 10:45 AM

Thanks a lot for this! Looks really great.

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
51

Only the interface method params need to have things fully qualified. Everything else (method bodies/extraTraitDecls/extraClassDecls/verify/etc.) only ever get placed in the mlir:: namespace.

219–220

You should be able to use $_op for this instead.

mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
98
100
mlir/lib/Analysis/BufferViewFlowAnalysis.cpp
106

nit: Cache the result as a variable and reuse it below.

116
mlir/lib/Analysis/DataFlowAnalysis.cpp
567

Can you add braces here? The body spans quite a few lines.

mlir/lib/Interfaces/ControlFlowInterfaces.cpp
198–199
213
231

When is operation ever null? dyn_cast (well isa) asserts on null, so this can't happen.

250

I mentioned it earlier, but I'd love if ReturnLike could imply RegionBranchTerminatorOpInterface somehow. That make this code much simpler to deal with (i.e. users could just interface with RegionBranchTerminatorOpInterface directly). Can you add a TODO here for exploring that?

This revision is now accepted and ready to land.Jul 22 2021, 10:45 AM
dfki-mako updated this revision to Diff 361146.Jul 23 2021, 3:15 AM
dfki-mako marked 11 inline comments as done.

Addressed reviewer comments from @rriddle.