[mlir][Transforms] OneToNTypeConversion.cpp: Fix invalid IR

[matthias-springer]

buildUnrealizedCast used to generate invalid builtin.unrealized_conversion_cast ops with zero results. This commit fixes test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir when running with MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS.

  * Pattern (anonymous namespace)::ConvertMakeTupleOp : 'test.make_tuple -> ()' {
Trying to match "(anonymous namespace)::ConvertMakeTupleOp"

[...]

"(anonymous namespace)::ConvertMakeTupleOp" result 1
  } -> success : pattern applied successfully
// *** IR Dump After Pattern Application ***
mlir-asm-printer: Verifying operation: func.func
'builtin.unrealized_conversion_cast' op expected at least one result for cast operation
mlir-asm-printer: 'func.func' failed to verify and will be printed in generic form
"func.func"() <{function_type = (i1, i2) -> (i1, i2), sym_name = "pack_unpack"}> ({
^bb0(%arg0: i1, %arg1: i2):
  %0 = "test.make_tuple"() : () -> tuple<>
  "builtin.unrealized_conversion_cast"(%0) {"__one-to-n-type-conversion_cast-kind__" = "target"} : (tuple<>) -> ()

[...]

}) : () -> ()

within split at /usr/local/google/home/springerm/mlir_public/llvm-project/mlir/test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir:1 offset :20:8: error: 'builtin.unrealized_conversion_cast' op expected at least one result for cast operation
  %0 = "test.make_tuple"() : () -> tuple<>
       ^
within split at /usr/local/google/home/springerm/mlir_public/llvm-project/mlir/test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir:1 offset :20:8: note: see current operation: "builtin.unrealized_conversion_cast"(%0) {"__one-to-n-type-conversion_cast-kind__" = "target"} : (tuple<>) -> ()
LLVM ERROR: IR failed to verify after pattern application

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

buildUnrealizedCast used to generate invalid builtin.unrealized_conversion_cast ops with zero results. This commit fixes test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir when running with MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS.

  * Pattern (anonymous namespace)::ConvertMakeTupleOp : 'test.make_tuple -> ()' {
Trying to match "(anonymous namespace)::ConvertMakeTupleOp"

[...]

"(anonymous namespace)::ConvertMakeTupleOp" result 1
  } -> success : pattern applied successfully
// *** IR Dump After Pattern Application ***
mlir-asm-printer: Verifying operation: func.func
'builtin.unrealized_conversion_cast' op expected at least one result for cast operation
mlir-asm-printer: 'func.func' failed to verify and will be printed in generic form
"func.func"() <{function_type = (i1, i2) -> (i1, i2), sym_name = "pack_unpack"}> ({
^bb0(%arg0: i1, %arg1: i2):
  %0 = "test.make_tuple"() : () -> tuple<>
  "builtin.unrealized_conversion_cast"(%0) {"__one-to-n-type-conversion_cast-kind__" = "target"} : (tuple<>) -> ()

[...]

}) : () -> ()

within split at /usr/local/google/home/springerm/mlir_public/llvm-project/mlir/test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir:1 offset :20:8: error: 'builtin.unrealized_conversion_cast' op expected at least one result for cast operation
  %0 = "test.make_tuple"() : () -> tuple<>
       ^
within split at /usr/local/google/home/springerm/mlir_public/llvm-project/mlir/test/Conversion/OneToNTypeConversion/one-to-n-type-conversion.mlir:1 offset :20:8: note: see current operation: "builtin.unrealized_conversion_cast"(%0) {"__one-to-n-type-conversion_cast-kind__" = "target"} : (tuple<>) -> ()
LLVM ERROR: IR failed to verify after pattern application

---

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

1 Files Affected:

• (modified) mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp (+3)

diff --git a/mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp b/mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp
index c95aa608636d8a..6ebc12903dd7c3 100644
--- a/mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp
+++ b/mlir/lib/Transforms/Utils/OneToNTypeConversion.cpp
@@ -113,6 +113,9 @@ static const char *const castKindAttrName =
 /// result types. Returns the result values of the cast.
 static ValueRange buildUnrealizedCast(OpBuilder &builder, TypeRange resultTypes,
                                       ValueRange inputs, CastKind kind) {
+  if (resultTypes.empty())
+    return ValueRange();
+
   // Create cast.
   Location loc = builder.getUnknownLoc();
   if (!inputs.empty())

[ingomueller-net]

Awesome work on MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS! Thanks for looking into the 1:n conversion as well!

Some questions, one small request:

• What's the plan for enabling MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS as part of CI? Should there not be a plan to enable it eventually, would there be any way to test the code otherwise?
• Can you confirm the following reasoning? The proposed fix is valid because if the previous result types are converted to "no result types", then there cannot be any consumers of the results, so no consumer need any cast. Furthermore, the cast is supposed to disappear anyways, so we wouldn't in any case rely on the fact that there is a user of the inputs either (for example, to prevent DCE).
• Could you add a brief sentence to the function documentation about this edge case?

[joker-eph]

What's the plan for enabling MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS as part of CI? Should there not be a plan to enable it eventually, would there be any way to test the code otherwise?

As soon as ninja check-mlir is clean, I'll turn it on on some of the bots.

[matthias-springer]

I've been fixing failures throughout the last weeks. We're almost there.

What's the plan for enabling MLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS as part of CI? Should there not be a plan to enable it eventually, would there be any way to test the code otherwise?

You can always enable these checks manually with this CMake option: -DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON. This will slow down the tests (check-mlir) considerably. They take about 5-8 minutes on my machine. I have not looked at performance so far, but we should be able to speed up the extra checks by being more selective about which IR is verified.

The proposed fix is valid because if the previous result types are converted to "no result types", then there cannot be any consumers of the results, so no consumer need any cast.

Correct.

There's one more tricky case in the 1-to-n conversion test cases. There are 3 patterns for converting function signatures, one for each of these ops: func.func, func.call, func.return. After applying the func.func pattern, the IR fails to verify because the func.call ops have not been updated yet. (But it verifies eventually, when the entire IR has been processed.) I don't know yet what to do with that. In a sense, the func.call verifier does not adhere to MLIR coding guidelines because it verifies a non-local property.

Note: We don't have this issue with the regular dialect conversion because there are no "expensive checks" to verify the IR after conversion pattern applications. We only do that for rewrite patterns.

[ingomueller-net]

Thanks for the explanation and confirmation!

There's one more tricky case in the 1-to-n conversion test cases. There are 3 patterns for converting function signatures, one for each of these ops: func.func, func.call, func.return. After applying the func.func pattern, the IR fails to verify because the func.call ops have not been updated yet. (But it verifies eventually, when the entire IR has been processed.) I don't know yet what to do with that. In a sense, the func.call verifier does not adhere to MLIR coding guidelines because it verifies a non-local property.

Note: We don't have this issue with the regular dialect conversion because there are no "expensive checks" to verify the IR after conversion pattern applications. We only do that for rewrite patterns.

Yeah, I understand the problem. At the time, I think I didn't fully understand it -- I just applied the same pattern as in the regular conversion and saw that it worked...

I am not sure if the problem is really the verifier of func.call. I think it doesn't actually verify the matching signatures itself. Instead, it implements 'SymbolUserOpInterface](https://github.com/llvm/llvm-project/blob/7851670/mlir/include/mlir/IR/SymbolInterfaces.td#L198) and thus [verifySymbolUses`, which is called by the containing symbol table op. In fact, all verification seems to verify local properties this way.

I rather have the feeling that one cannot expect the IR to (fully?) verify while pattern application is still ongoing. Otherwise, symbols and all of their uses would always have to be changed by the same pattern, which clearly isn't what's happening, right?