-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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); | ||||
Comment on lines
+1077
to
+1079
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather avoid iterating over the use-def list every time... This goes through block's use-def chain, maybe there is a way to expose a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think so
Compare with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry just to add a litlle more "context"; if you look at 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 commentThe reason will be displayed to describe this comment to others. Learn more. Define 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 commentThe reason will be displayed to describe this comment to others. Learn more. The problem is there's no way to implement https://github.com/llvm/llvm-project/blob/main/mlir/lib/CAPI/IR/IR.cpp#L969 without holding an instance of 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 commentThe reason will be displayed to describe this comment to others. Learn more. added warning |
||||
} | ||||
|
||||
//===----------------------------------------------------------------------===// | ||||
// Value API. | ||||
//===----------------------------------------------------------------------===// | ||||
|
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