Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir] NFC: void visitRegionSuccessors #125268

Merged

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Jan 31, 2025

I would like to hook/override visitRegionSuccessors in sparse analyses that inherit from AbstractSparseForwardDataFlowAnalysis. This would allow me to control specifically how regions are analyzed (e.g., order and number of visits).

cc @antiagainst

@makslevental makslevental requested a review from Mogball January 31, 2025 18:57
@llvmbot llvmbot added the mlir label Jan 31, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 31, 2025

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

I would like to hook/override visitRegionSuccessors in sparse analyses that inherit from AbstractSparseForwardDataFlowAnalysis. This would allow me to control specifically how regions are analyzed (e.g., number of visits).


Full diff: https://github.com/llvm/llvm-project/pull/125268.diff

1 Files Affected:

  • (modified) mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h (+1-1)
diff --git a/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h b/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
index 387b9ee707179bf..8c67c9dd75de598 100644
--- a/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
@@ -255,7 +255,7 @@ class AbstractSparseForwardDataFlowAnalysis : public DataFlowAnalysis {
   /// operation `branch`, which can either be the entry block of one of the
   /// regions or the parent operation itself, and set either the argument or
   /// parent result lattices.
-  void visitRegionSuccessors(ProgramPoint *point,
+  virtual void visitRegionSuccessors(ProgramPoint *point,
                              RegionBranchOpInterface branch,
                              RegionBranchPoint successor,
                              ArrayRef<AbstractSparseLattice *> lattices);

Copy link

github-actions bot commented Jan 31, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@makslevental makslevental force-pushed the makslevental/virtual-visitRegionSuccessor branch from 1ca6b56 to cf452cc Compare January 31, 2025 19:03
@joker-eph
Copy link
Collaborator

Making a method virtual is not a trivial change: it at minima deserves to update the document IMO.

In particular the dataflow framework is already a complex piece of of machinery that is hard to grasp, every new customization point increases the complexity, and should be described in terms of the invariant of the system (what is an implemented overriding this allowed to do here and for which purposes?).

This PR enables overriding AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors to control precisely how the region successors of `branch` are visited. For example in order to precisely control the order in which predecessor operand lattices are propagated from. An override is responsible for visiting all the known predecessors and propagating therefrom.
@makslevental makslevental force-pushed the makslevental/virtual-visitRegionSuccessor branch from cf452cc to e02efa6 Compare January 31, 2025 19:39
@makslevental
Copy link
Contributor Author

Add a note in the doc string (and the commit mesage) about invariants/responsibilities of overriding method.

@makslevental makslevental merged commit ffe3129 into llvm:main Jan 31, 2025
8 checks passed
@makslevental makslevental deleted the makslevental/virtual-visitRegionSuccessor branch January 31, 2025 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants