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

[victor-eds]

Change SingleBlock::{insert,push_back} to avoid inserting the argument operation after the block's terminator. This allows removing SingleBlockImplicitTerminator's functions with the same name.

Define Block::hasTerminator checking whether the block has a terminator operation.

Signed-off-by: Victor Perez victor.perez@codeplay.com

[llvmbot]

@llvm/pr-subscribers-mlir

Changes

Avoid member functions duplication caused by SingleBlockImplicitTerminator now implying SingleBlock (since 0ac21e6).

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

1 Files Affected:

• (modified) mlir/include/mlir/IR/OpDefinition.h (-31)

diff --git a/mlir/include/mlir/IR/OpDefinition.h b/mlir/include/mlir/IR/OpDefinition.h
index 84ba46f4d6f3ec1..fade95f200476f2 100644
--- a/mlir/include/mlir/IR/OpDefinition.h
+++ b/mlir/include/mlir/IR/OpDefinition.h
@@ -996,37 +996,6 @@ struct SingleBlockImplicitTerminator {
       ::mlir::impl::ensureRegionTerminator(region, builder, loc,
                                            buildTerminator);
     }
-
-    //===------------------------------------------------------------------===//
-    // Single Region Utilities
-    //===------------------------------------------------------------------===//
-
-    template 
-    using enable_if_single_region =
-        std::enable_if_t(), T>;
-
-    /// Insert the operation into the back of the body, before the terminator.
-    template 
-    enable_if_single_region push_back(Operation *op) {
-      Block *body = static_cast *>(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 
-    enable_if_single_region insert(Operation *insertPt, Operation *op) {
-      insert(Block::iterator(insertPt), op);
-    }
-    template 
-    enable_if_single_region insert(Block::iterator insertPt,
-                                        Operation *op) {
-      Block *body = static_cast *>(this)->getBody();
-      if (insertPt == body->end())
-        insertPt = Block::iterator(body->getTerminator());
-      body->getOperations().insert(insertPt, op);
-    }
   };
 };
 

[victor-eds]

This causes build failures if the member functions are used.

[victor-eds]

@matthias-springer updated according to Discourse conversation

[joker-eph]

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

[joker-eph]

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.

[matthias-springer]

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.

[joker-eph]

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.

[victor-eds]

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)

[joker-eph]

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?

[victor-eds]

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

[joker-eph]

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.

[victor-eds]

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.

[joker-eph]

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.

[victor-eds]

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