Skip to content

[mlir][IR] Insert operations before SingleBlock's terminator #65959

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 15, 2023
Merged

[mlir][IR] Insert operations before SingleBlock's terminator #65959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions mlir/include/mlir/IR/Block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ class Block : public IRObjectWithUseList<BlockOperand>,
/// the block has a valid terminator operation.
Operation *getTerminator();

/// Check whether this block has a terminator.
bool hasTerminator();

//===--------------------------------------------------------------------===//
// Predecessors and successors.
//===--------------------------------------------------------------------===//
Expand Down
35 changes: 4 additions & 31 deletions mlir/include/mlir/IR/OpDefinition.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,10 @@ struct SingleBlock : public TraitBase<ConcreteType, SingleBlock> {
}
template <typename OpT = ConcreteType>
enable_if_single_region<OpT> insert(Block::iterator insertPt, Operation *op) {
Block *body = getBody();
// Insert op before the block's terminator if it has one
if (insertPt == body->end() && body->hasTerminator())
insertPt = Block::iterator(body->getTerminator());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a safe: if there is an unregistered operation at the end of the current block it'll insert before.

I don't see a safe way of doing this actually.

Copy link
Member

@matthias-springer matthias-springer Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, (before 0ac21e6, which made push_back ambiguous) this was actually (somewhat?) safe. SingleBlockImplicitTerminator<OpTy> must be instantiated with an op type, which means that the terminator op must be known (registered?).

I'm wondering if it's better to just always insert at the end of the block. It would simplify the API: no special handling for terminators, it behaves like a vector::push_back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit: the terminator must be known when the verifier is passing, this may not be the case in the middle of a transformation or while building the operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the definition of hasTerminator so that it checks the last operation is definitely a terminator and keeping current semantics should work, right? (of course reverting the change to the assertion in this PR)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me that it'll fix it: if you can't be sure if the last op is a terminator or not, I don't quite see how you can decide to insert before or after it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is: if it is definitely a terminator, do not break the code and insert before it; keep vector::push_back semantics otherwise

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends if the operation is registered in the MLIRContext, so we're creating a different behavior here, and avoiding this is the reason the "mighHaveTrait" exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do you guys think we should just be consistent with vector::push_back semantics and just blame the API user if the code is broken? I'm fine with that, just wanted to keep former semantics.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The push_back approach has the advantage of being consistent in terms of behavior (there is no "gotcha" involved). If there is an API misuse here it'll blow up immediately and consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense. I'll push a follow-up PR.

getBody()->getOperations().insert(insertPt, op);
}
};
Expand Down Expand Up @@ -996,37 +1000,6 @@ struct SingleBlockImplicitTerminator {
::mlir::impl::ensureRegionTerminator(region, builder, loc,
buildTerminator);
}

//===------------------------------------------------------------------===//
// Single Region Utilities
//===------------------------------------------------------------------===//

template <typename OpT, typename T = void>
using enable_if_single_region =
std::enable_if_t<OpT::template hasTrait<OneRegion>(), T>;

/// Insert the operation into the back of the body, before the terminator.
template <typename OpT = ConcreteType>
enable_if_single_region<OpT> push_back(Operation *op) {
Block *body = static_cast<SingleBlock<ConcreteType> *>(this)->getBody();
insert(Block::iterator(body->getTerminator()), op);
}

/// Insert the operation at the given insertion point. Note: The operation
/// is never inserted after the terminator, even if the insertion point is
/// end().
template <typename OpT = ConcreteType>
enable_if_single_region<OpT> insert(Operation *insertPt, Operation *op) {
insert(Block::iterator(insertPt), op);
}
template <typename OpT = ConcreteType>
enable_if_single_region<OpT> insert(Block::iterator insertPt,
Operation *op) {
Block *body = static_cast<SingleBlock<ConcreteType> *>(this)->getBody();
if (insertPt == body->end())
insertPt = Block::iterator(body->getTerminator());
body->getOperations().insert(insertPt, op);
}
};
};

Expand Down
7 changes: 6 additions & 1 deletion mlir/lib/IR/Block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,15 @@ void Block::eraseArguments(function_ref<bool(BlockArgument)> shouldEraseFn) {
/// Get the terminator operation of this block. This function asserts that
/// the block has a valid terminator operation.
Operation *Block::getTerminator() {
assert(!empty() && back().mightHaveTrait<OpTrait::IsTerminator>());
assert(hasTerminator());
return &back();
}

/// Check whether this block has a terminator.
bool Block::hasTerminator() {
return !empty() && back().mightHaveTrait<OpTrait::IsTerminator>();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API is misnamed: this should be "mightHaveTerminator", as the trait check indicates.


// Indexed successor access.
unsigned Block::getNumSuccessors() {
return empty() ? 0 : back().getNumSuccessors();
Expand Down