[mlir][IR] Fix Block::without_terminator for blocks without terminator (#154498)

Blocks without a terminator are not handled correctly by
`Block::without_terminator`: the last operation is excluded, even when
it is not a terminator. With this commit, only terminators are excluded.
If the last operation is unregistered, it is included for safety.

Author: matthias-springer

mlir/include/mlir/IR/Block.h

@@ -205,12 +205,14 @@ class alignas(8) Block : public IRObjectWithUseList<BlockOperand>,
   }
 
   /// Return an iterator range over the operation within this block excluding
-  /// the terminator operation at the end.
+  /// the terminator operation at the end. If the block has no terminator,
+  /// return an iterator range over the entire block. If it is unknown if the
+  /// block has a terminator (i.e., last block operation is unregistered), also
+  /// return an iterator range over the entire block.
   iterator_range<iterator> without_terminator() {
     if (begin() == end())
       return {begin(), end()};
-    auto endIt = --end();
-    return {begin(), endIt};
+    return without_terminator_impl();
   }
 
   //===--------------------------------------------------------------------===//
@@ -221,7 +223,8 @@ class alignas(8) Block : public IRObjectWithUseList<BlockOperand>,
   /// the block might have a valid terminator operation.
   Operation *getTerminator();
 
-  /// Check whether this block might have a terminator.
+  /// Return "true" if this block might have a terminator. Return "true" if
+  /// the last operation is unregistered.
   bool mightHaveTerminator();
 
   //===--------------------------------------------------------------------===//
@@ -402,6 +405,9 @@ class alignas(8) Block : public IRObjectWithUseList<BlockOperand>,
   void printAsOperand(raw_ostream &os, AsmState &state);
 
 private:
+  /// Same as `without_terminator`, but assumes that the block is not empty.
+  iterator_range<iterator> without_terminator_impl();
+
   /// Pair of the parent object that owns this block and a bit that signifies if
   /// the operations within this block have a valid ordering.
   llvm::PointerIntPair<Region *, /*IntBits=*/1, bool> parentValidOpOrderPair;

mlir/lib/Analysis/TopologicalSortUtils.cpp

@@ -101,12 +101,7 @@ bool mlir::sortTopologically(
 
 bool mlir::sortTopologically(
     Block *block, function_ref<bool(Value, Operation *)> isOperandReady) {
-  if (block->empty())
-    return true;
-  if (block->back().hasTrait<OpTrait::IsTerminator>())
-    return sortTopologically(block, block->without_terminator(),
-                             isOperandReady);
-  return sortTopologically(block, *block, isOperandReady);
+  return sortTopologically(block, block->without_terminator(), isOperandReady);
 }
 
 bool mlir::computeTopologicalSorting(

mlir/lib/IR/Block.cpp

@@ -251,6 +251,16 @@ bool Block::mightHaveTerminator() {
   return !empty() && back().mightHaveTrait<OpTrait::IsTerminator>();
 }
 
+iterator_range<Block::iterator> Block::without_terminator_impl() {
+  // Note: When the op is unregistered, we do not know for sure if the last
+  // op is a terminator. In that case, we include it in `without_terminator`,
+  // but that decision is somewhat arbitrary.
+  if (!back().hasTrait<OpTrait::IsTerminator>())
+    return {begin(), end()};
+  auto endIt = --end();
+  return {begin(), endIt};
+}
+
 // Indexed successor access.
 unsigned Block::getNumSuccessors() {
   return empty() ? 0 : back().getNumSuccessors();