[Mlir-commits] [mlir] [mlir][func] Move return-type verification from ReturnOp to FuncOp (PR #184153)

[Matthias Springer]

================
@@ -284,23 +284,37 @@ FuncOp FuncOp::clone() {
 // ReturnOp
 //===----------------------------------------------------------------------===//
 
-LogicalResult ReturnOp::verify() {
-  auto function = cast<FuncOp>((*this)->getParentOp());
-
-  // The operand number and types must match the function signature.
-  const auto &results = function.getFunctionType().getResults();
-  if (getNumOperands() != results.size())
-    return emitOpError("has ")
-           << getNumOperands() << " operands, but enclosing function (@"
-           << function.getName() << ") returns " << results.size();
-
-  for (unsigned i = 0, e = results.size(); i != e; ++i)
-    if (getOperand(i).getType() != results[i])
-      return emitError() << "type of return operand " << i << " ("
-                         << getOperand(i).getType()
-                         << ") doesn't match function result type ("
-                         << results[i] << ")"
-                         << " in function @" << function.getName();
+LogicalResult FuncOp::verify() {
+  // External declarations have no body to check.
+  if (isDeclaration())
+    return success();
+  // Hoist the result types once; they are the same for every return site.
+  auto resultTypes = getFunctionType().getResults();
+  for (Block &block : getBody()) {
+    if (block.empty())
+      continue;
+    // Check func.return or other return-like terminators ops (e.g.
+    // llvm.return, test.return).
+    auto returnOp = dyn_cast<RegionBranchTerminatorOpInterface>(&block.back());
----------------
matthias-springer wrote:

There is a problem with `RegionBranchTerminatorOpInterface`.

- The verifier assumes that all operands are returned (we call `returnOp->getNumOperands()`). That may not be the case. There is `RegionBranchTerminatorOpInterface::getMutableSuccessorOperands()`.
- The `RegionBranchOpInterface` has the concept of "produced operands". In this particular case: values that are not explicitly passed as operands but produced "internally" by the operations. These are not accounted for in the current implementation. And there is no API to query this information for a `func.func`. You would normally call `RegionBranchOpInterface::getSuccessorInputs()` to query only the "forwarded" op results (everything else is "produced"), but `func.func` does not have any op results.

This makes me wonder if it's better to leave the verification of return types in `func.return`, where we know (from the op semantics) that it does not have any produced operands. 

https://github.com/llvm/llvm-project/pull/184153