-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[mlir][Affine][NFC] Amortize cost of block op index lookups #156027
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
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
27909ee
to
9535e8b
Compare
@llvm/pr-subscribers-mlir Author: Samarth Narang (snarang181) ChangesThis patch addresses the Full diff: https://github.com/llvm/llvm-project/pull/156027.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
index 99ea20bf13b49..c9e1ef9781af7 100644
--- a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
+++ b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
@@ -1442,16 +1442,28 @@ template LogicalResult
mlir::affine::boundCheckLoadOrStoreOp(AffineWriteOpInterface storeOp,
bool emitError);
+static inline unsigned getIndexInBlock(
+ Operation *op,
+ llvm::DenseMap<Block *, llvm::DenseMap<Operation *, unsigned>> &cache) {
+ Block *block = op->getBlock();
+ auto &blockMap = cache[block];
+ if (blockMap.empty()) {
+ unsigned idx = 0;
+ for (Operation &it : *block)
+ blockMap[&it] = idx++;
+ }
+ return blockMap.lookup(op);
+}
+
// Returns in 'positions' the Block positions of 'op' in each ancestor
// Block from the Block containing operation, stopping at 'limitBlock'.
static void findInstPosition(Operation *op, Block *limitBlock,
SmallVectorImpl<unsigned> *positions) {
+ llvm::DenseMap<Block *, llvm::DenseMap<Operation *, unsigned>> indexCache;
+
Block *block = op->getBlock();
while (block != limitBlock) {
- // FIXME: This algorithm is unnecessarily O(n) and should be improved to not
- // rely on linear scans.
- int instPosInBlock = std::distance(block->begin(), op->getIterator());
- positions->push_back(instPosInBlock);
+ positions->push_back(getIndexInBlock(op, indexCache));
op = block->getParentOp();
block = op->getBlock();
}
|
@llvm/pr-subscribers-mlir-affine Author: Samarth Narang (snarang181) ChangesThis patch addresses the Full diff: https://github.com/llvm/llvm-project/pull/156027.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
index 99ea20bf13b49..c9e1ef9781af7 100644
--- a/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
+++ b/mlir/lib/Dialect/Affine/Analysis/Utils.cpp
@@ -1442,16 +1442,28 @@ template LogicalResult
mlir::affine::boundCheckLoadOrStoreOp(AffineWriteOpInterface storeOp,
bool emitError);
+static inline unsigned getIndexInBlock(
+ Operation *op,
+ llvm::DenseMap<Block *, llvm::DenseMap<Operation *, unsigned>> &cache) {
+ Block *block = op->getBlock();
+ auto &blockMap = cache[block];
+ if (blockMap.empty()) {
+ unsigned idx = 0;
+ for (Operation &it : *block)
+ blockMap[&it] = idx++;
+ }
+ return blockMap.lookup(op);
+}
+
// Returns in 'positions' the Block positions of 'op' in each ancestor
// Block from the Block containing operation, stopping at 'limitBlock'.
static void findInstPosition(Operation *op, Block *limitBlock,
SmallVectorImpl<unsigned> *positions) {
+ llvm::DenseMap<Block *, llvm::DenseMap<Operation *, unsigned>> indexCache;
+
Block *block = op->getBlock();
while (block != limitBlock) {
- // FIXME: This algorithm is unnecessarily O(n) and should be improved to not
- // rely on linear scans.
- int instPosInBlock = std::distance(block->begin(), op->getIterator());
- positions->push_back(instPosInBlock);
+ positions->push_back(getIndexInBlock(op, indexCache));
op = block->getParentOp();
block = op->getBlock();
}
|
Pinging reviewers to review, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this cache is effective. The cache is re-created every time we enter the findInstPosition
function. The function traverses ancestor blocks for the given op once and then exists. There doesn't seem to be any reuse. FWIW, this is worse because the logic always goes until the end of each block and creates data structures, as opposed to going only until the (ancestor of the) op in question.
I suspect the comment may have either referred to using the cached position of the operation (
llvm-project/mlir/include/mlir/IR/Operation.h
Line 1047 in 50f539c
mutable unsigned orderIndex = 0; |
isBeforeInBlock
, or a larger overhaul of the algorithm that doesn't need numeric positions of an operation in the block.
This patch addresses the
O(n)
scan as it relied onstd::distance(block->begin(), op->getIterator())
, which will perform a linear scan of the block each time.This patch builds a per-block
Operation* -> index
map, which reduces subsequent lookups in the same block toO(1)
.