This is an archive of the discontinued LLVM Phabricator instance.

[mlir] allow dense dataflow to customize call and region operations
ClosedPublic

Authored by ftynse on Jul 19 2023, 3:06 PM.

Details

Summary

Initial implementations of dense dataflow analyses feature special cases
for operations that have region- or call-based control flow by
leveraging the corresponding interfaces. This is not necessarily
sufficient as these operations may influence the dataflow state by
themselves as well we through the control flow. For example,
linalg.generic and similar operations have region-based control flow
and their proper memory effects, so any memory-related analyses such as
last-writer require processing linalg.generic directly instead of, or
in addition to, the region-based flow.

Provide hooks to customize the processing of operations with region-
cand call-based contol flow in forward and backward dense dataflow
analysis. These hooks are trigerred when control flow is transferred
between the "main" operation, i.e. the call or the region owner, and
another region. Such an apporach allows the analyses to update the
lattice before and/or after the regions. In the linalg.generic
example, the reads from memory are interpreted as happening before the
body region and the writes to memory are interpreted as happening after
the body region. Using these hooks in generic analysis may require
introducing additional interfaces, but for now assume that the specific
analysis have spceial cases for the (rare) operaitons with call- and
region-based control flow that need additional processing.

Diff Detail

Event Timeline

ftynse created this revision.Jul 19 2023, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 3:06 PM
ftynse requested review of this revision.Jul 19 2023, 3:06 PM
ftynse updated this revision to Diff 542524.Jul 20 2023, 7:51 AM

Keep visitRegionBranchOperation as protected because flang uses it.

Mogball accepted this revision.Jul 20 2023, 7:59 AM
Mogball added inline comments.
mlir/include/mlir/Analysis/DataFlow/DenseAnalysis.h
135
mlir/lib/Analysis/DataFlow/DenseAnalysis.cpp
59

Unused?

This revision is now accepted and ready to land.Jul 20 2023, 7:59 AM
phisiart accepted this revision.Jul 20 2023, 9:27 PM
phisiart added inline comments.
mlir/lib/Analysis/DataFlow/DenseAnalysis.cpp
56

The APIs involved here are rather complex. Let's make variable names more verbose for clarification.

void AbstractDenseDataFlowAnalysis::visitCallOperation(
    CallOpInterface call, AbstractDenseLattice *latticeAfterCall) {
  ...
  for (Operation *predecessor : predecessors->getKnownPredecessors()) {
    // func.func @callee() {
    //   ...
    //   return  // predecessor
    //   // latticeAtCalleeReturn
    // }
    // func.func @caller() {
    //   ...
    //   call @callee
    //   // latticeAfterCall
    //   ...
    // }
    const auto *latticeAtCalleeReturn = getLatticeFor(call.getOperation(), predecessor);
    visitCallControlFlowTransfer(call, CallControlFlowAction::ExitCallee,
                                 *latticeAtCalleeReturn, latticeAfterCall);
  }
}
213

/*regionTo=*/

275–280

Similarly, let's make variable names more verbose for clarification.

C
void AbstractDenseBackwardDataFlowAnalysis::visitCallOperation(
    CallOpInterface call, AbstractDenseLattice *latticeBeforeCall) {
  ...
  // func.func @callee() {
  //   ^calleeEntryBlock:
  //   // latticeAtCalleeEntry
  //   ...
  // }
  // func.func @caller() {
  //   ...
  //   // latticeBeforeCall
  //   call @callee
  //   ...
  // }
  Block *calleeEntryBlock = &region->front();
  ProgramPoint calleeEntry = calleeEntryBlock->empty() ?
                             ProgramPoint(entryBlock) :
                             &calleeEntryBlock->front();
  const AbstractDenseLattice &latticeAtCalleeEntry = *getLatticeFor(calleeEntry);
  const auto *latticeAtCalleeReturn = getLatticeFor(call.getOperation(), predecessor);
  visitCallControlFlowTransfer(call, CallControlFlowAction::EnterCallee,
                               *latticeAtCalleeEntry, latticeBeforeCall);
}
ftynse updated this revision to Diff 542808.Jul 21 2023, 2:06 AM
ftynse marked 5 inline comments as done.

Address review.

This revision was landed with ongoing or failed builds.Jul 21 2023, 2:16 AM
This revision was automatically updated to reflect the committed changes.

Hi, @ftynse,

I think this patch may have introduced a regression when trying to analyze loop-like RegionBranchTerminatorOpInterface ops. Applying this patch:

diff
diff --git a/mlir/test/lib/Dialect/Test/TestDialect.cpp b/mlir/test/lib/Dialect/Test/TestDialect.cpp
index 485d21823eb6..640ff8f8e669 100644
--- a/mlir/test/lib/Dialect/Test/TestDialect.cpp
+++ b/mlir/test/lib/Dialect/Test/TestDialect.cpp
@@ -1273,6 +1273,7 @@ void TestStoreWithARegion::getSuccessorRegions(
   if (!index) {
     regions.emplace_back(&getBody(), getBody().front().getArguments());
   } else {
+    regions.emplace_back(&getBody(), getBody().front().getArguments());
     regions.emplace_back();
   }
 }

and running: test-last-modified.mlir causes assertion op == branch || toBlock->getParent() != op->getBlock()->getParent() to fail.

What would the best solution be in your opinion? Is removing the assertion enough or does that violate any relevant precondition?

ftynse added a comment.Sep 4 2023, 5:53 AM

At the time of this patch, there were some quirks with the kinds of control flow that were supported by RegionBranchOpInterface, this is likely one of them. The quirks have been fixed since then so I suppose just removing the assertion is okay. Please submit a patch (ideally via Github).