[MLIR] Flag no-terminator error on the last operation of non-empty blocks

[jurahul]

Summary

• When a block is not empty and does not end with a terminator, flag the error on the last operation of the block instead of the start of the block.

[mehdi_amini]

mlir/test/IR/invalid.mlir

175

Can you provide a sample of the output before and after the code change on this example?

[jurahul]

mlir/test/IR/invalid.mlir

175

The old example does not reproduce the issue. With the new example and no test MLIR changes, the error is flagged on @x

invalid.mlir split at line #171:4:8: error: block with no terminator
  %x = constant 0 : i32

With the change, its flagged on @y

invalid.mlir split at line #171:5:8: error: block with no terminator
  %y = constant 1 : i32

The last operation is not the most ideal, but much better than the first operation (of potentially long block) to indicate this error.

[mehdi_amini]

mlir/test/IR/invalid.mlir

175

Yeah I mean the new example, thanks!

[rriddle]

FWIW I see this going either way. The first operation can often be a better location given that it is much closer to the when/where the block was created.

[stephenneuendorffer]

This still seems confusing, since the operation itself may have a region with only one block. Maybe we could show a range of line numbers from the first and last instructions in the block?

[jurahul]

I already push this in, but we can revisit this if there is a better way. Could you give an example where the confusion arises? To me, the last instruction seems more relevant because that's where something is missing. Are you suggesting doing:

if (block.back().isKnownNonTerminator()) {
  block.emitError("block with no terminator");
  return block.back().emitError("block with no terminator");
}

i.e., emit the error twice?

mlir/lib/IR/Verifier.cpp

for (auto &op : llvm::make_range(block.begin(), std::prev(block.end()))) {
     if (failed(verifyOperation(op)))
       return failure();
   }
 
   // Verify the terminator.
   if (failed(verifyOperation(block.back())))
     return failure();
   if (block.back().isKnownNonTerminator())
-    return emitError(block, "block with no terminator");
+    return block.back().emitError("block with no terminator");
 
   // Verify that this block is not branching to a block of a different
   // region.
   for (Block *successor : block.getSuccessors())
     if (successor->getParent() != block.getParent())
       return block.back().emitOpError(
           "branching to block of a different region");
 

mlir/test/IR/invalid.mlir

^bb42:
   br ^bb43
 ^bb43:
   br ^bb42
 }
 
 // -----
 
 func @no_return() {
-  %x = constant 0 : i32  // expected-error {{block with no terminator}}
+  %x = constant 0 : i32
+  %y = constant 1 : i32  // expected-error {{block with no terminator}}
 }
 
 // -----
 
 "       // expected-error {{expected}}
 "
 
 // -----