[Mlir-commits] [mlir] [MLIR][Toy] Fix incorrect return value in PrintExprAST codegen (PR #226924)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: vaachang

<details>
<summary>Changes</summary>

This PR fixes a logic error in the Toy tutorial's `MLIRGen.cpp` where a failed
`toy.print` code generation incorrectly returned `mlir::success()` instead of
`mlir::failure()`.

### Problem

In `mlirGen(ExprASTList &blockAST)`, the code currently reads:

    if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
      if (mlir::failed(mlirGen(*print)))
        return mlir::success();
      continue;
    }

When `mlirGen(*print)` fails (e.g., due to an invalid argument or an internal
error), `mlir::failed(...)` is `true`. However, the function returns
`mlir::success()`, which tells the caller that the whole block was generated
successfully. As a result, `mlirGen(FunctionAST&)` does not erase the invalid
function and does not return `nullptr`. The module may later fail verification,
but the original error is masked.

### Fix

Change the erroneous `return mlir::success();` to `return mlir::failure();`:

    if (mlir::failed(mlirGen(*print)))
      return mlir::failure();

This makes the error handling consistent with the other failure paths in the
same function (e.g., variable declarations and generic expressions).

### Test

No new test is added because this is a straightforward logic fix in an example
tutorial. The change simply propagates the failure correctly; the existing
behavior for successful code generation is unchanged.

### Additional context

Found while reviewing the Toy tutorial code. The same pattern is already
correct in other error paths within the same file.

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


6 Files Affected:

- (modified) mlir/examples/toy/Ch2/mlir/MLIRGen.cpp (+1-1) 
- (modified) mlir/examples/toy/Ch3/mlir/MLIRGen.cpp (+1-1) 
- (modified) mlir/examples/toy/Ch4/mlir/MLIRGen.cpp (+1-1) 
- (modified) mlir/examples/toy/Ch5/mlir/MLIRGen.cpp (+1-1) 
- (modified) mlir/examples/toy/Ch6/mlir/MLIRGen.cpp (+1-1) 
- (modified) mlir/examples/toy/Ch7/mlir/MLIRGen.cpp (+1-1) 


``````````diff
diff --git a/mlir/examples/toy/Ch2/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch2/mlir/MLIRGen.cpp
index a9592bcd7814a..e44a0977cd637 100644
--- a/mlir/examples/toy/Ch2/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch2/mlir/MLIRGen.cpp
@@ -416,7 +416,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 
diff --git a/mlir/examples/toy/Ch3/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch3/mlir/MLIRGen.cpp
index 8c21951948496..a550ceef1af9d 100644
--- a/mlir/examples/toy/Ch3/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch3/mlir/MLIRGen.cpp
@@ -416,7 +416,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 
diff --git a/mlir/examples/toy/Ch4/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch4/mlir/MLIRGen.cpp
index 6b7ab40299be5..644b706098114 100644
--- a/mlir/examples/toy/Ch4/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch4/mlir/MLIRGen.cpp
@@ -420,7 +420,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 
diff --git a/mlir/examples/toy/Ch5/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch5/mlir/MLIRGen.cpp
index 6b7ab40299be5..644b706098114 100644
--- a/mlir/examples/toy/Ch5/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch5/mlir/MLIRGen.cpp
@@ -420,7 +420,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 
diff --git a/mlir/examples/toy/Ch6/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch6/mlir/MLIRGen.cpp
index 6b7ab40299be5..644b706098114 100644
--- a/mlir/examples/toy/Ch6/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch6/mlir/MLIRGen.cpp
@@ -420,7 +420,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 
diff --git a/mlir/examples/toy/Ch7/mlir/MLIRGen.cpp b/mlir/examples/toy/Ch7/mlir/MLIRGen.cpp
index 73133247335dc..c72df8890b38d 100644
--- a/mlir/examples/toy/Ch7/mlir/MLIRGen.cpp
+++ b/mlir/examples/toy/Ch7/mlir/MLIRGen.cpp
@@ -640,7 +640,7 @@ class MLIRGenImpl {
         return mlirGen(*ret);
       if (auto *print = dyn_cast<PrintExprAST>(expr.get())) {
         if (mlir::failed(mlirGen(*print)))
-          return mlir::success();
+          return mlir::failure();
         continue;
       }
 

``````````

</details>


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