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

[Ingo Müller]

================
@@ -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());
+    if (!returnOp)
+      continue;
+
+    if (returnOp->getNumOperands() != resultTypes.size())
+      return returnOp->emitOpError("has ")
+             << returnOp->getNumOperands()
+             << " operands, but enclosing function (@" << getName()
+             << ") returns " << resultTypes.size();
+
+    for (auto [i, opType] :
+         llvm::enumerate(llvm::zip(returnOp->getOperandTypes(), resultTypes))) {
----------------
ingomueller-net wrote:

I finally managed to narrow down the crash in the `tf-reduce` test: The reducer calls `dropAllUses` on the operand of a `func.return` and then tries to verify the resulting module. At that point, the `func.return` op has a `<<NULL>>` value as an operand. Curiously enough, this isn't caught before this line, so `returnOp->getOperandTypes()` tries to `getType` of a `nullptr`, which crashes.

I have a local fix that iterates `getOpOperands()` instead of `getOperandTypes()` and verifies that none of them is a `nullptr`. However, that may only be treating symptoms locally rather than addressing a potentially underlying broader problem. Why don't we normally need to verify that operands exist and why does that fail here? It seems to me like this code assumes that the `returnOp` has already been verified but that isn't the case. Is that so? Any way to verify it here?

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