-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[mlir][python] bind block predecessors and successors #145116
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
Changes from all commits
c760955
035e009
c5787ec
006d927
198b74d
7ae6d4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -1059,6 +1059,26 @@ void mlirBlockPrint(MlirBlock block, MlirStringCallback callback, | |||
| unwrap(block)->print(stream); | ||||
| } | ||||
|
|
||||
| intptr_t mlirBlockGetNumSuccessors(MlirBlock block) { | ||||
| return static_cast<intptr_t>(unwrap(block)->getNumSuccessors()); | ||||
| } | ||||
|
|
||||
| MlirBlock mlirBlockGetSuccessor(MlirBlock block, intptr_t pos) { | ||||
| return wrap(unwrap(block)->getSuccessor(static_cast<unsigned>(pos))); | ||||
| } | ||||
|
|
||||
| intptr_t mlirBlockGetNumPredecessors(MlirBlock block) { | ||||
| Block *b = unwrap(block); | ||||
| return static_cast<intptr_t>(std::distance(b->pred_begin(), b->pred_end())); | ||||
| } | ||||
|
|
||||
| MlirBlock mlirBlockGetPredecessor(MlirBlock block, intptr_t pos) { | ||||
| Block *b = unwrap(block); | ||||
| Block::pred_iterator it = b->pred_begin(); | ||||
| std::advance(it, pos); | ||||
| return wrap(*it); | ||||
|
||||
| /// Implement a predecessor iterator for blocks. This works by walking the use |
Compare with SuccessorRange just below there. But maybe I'm wrong and it's just not clicking for me.
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.
Sorry just to add a litlle more "context"; if you look at getSuccessors and getSuccessor(unsigned) they work differently but still in a way that doesn't seem possible for getPredecessors:
SuccessorRange getSuccessors() { return SuccessorRange(this); }
...
SuccessorRange::SuccessorRange(Block *block) : SuccessorRange() {
if (block->empty() || llvm::hasSingleElement(*block->getParent()))
return;
Operation *term = &block->back();
if ((count = term->getNumSuccessors()))
base = term->getBlockOperands().data();
}and
Block *Block::getSuccessor(unsigned i) {
assert(i < getNumSuccessors());
return getTerminator()->getSuccessor(i);
}
...
class Operation {
...
Block *getSuccessor(unsigned index) {
assert(index < getNumSuccessors());
return getBlockOperands()[index].get();
}
...
}compared with
using pred_iterator = PredecessorIterator;
pred_iterator pred_begin() {
return pred_iterator((BlockOperand *)getFirstUse());
}
pred_iterator pred_end() { return pred_iterator(nullptr); }
iterator_range<pred_iterator> getPredecessors() {
return {pred_begin(), pred_end()};
}so while I agree that iterating the chain isn't great I don't see what else can be done (other than caching those predecessors, which I'm sure we don't want to do either).
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.
Define predecessor as returning an iterable object that only has __next__?
Not a big problem if we don't, we use indexed accessors for the linked list of blocks as well because I thought it whoever was using Python didn't care about that level of performance tweaking.
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.
The problem is there's no way to implement GetNextPredecessor like GetNextBlockInRegion
https://github.com/llvm/llvm-project/blob/main/mlir/lib/CAPI/IR/IR.cpp#L969
without holding an instance of PredecessorIterator (even forgetting that it needs to be mapped into C).
Anyway ya I'm gonna leave this as is but I'll add a comment mentioning that it's expensive.
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.
added warning
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.
Let's have tests for the C API as well
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.
added a test