[Mlir-commits] [mlir] [mlir] fix a segment fault in `ConversionPatternRewriter::applySignatureConversion` (PR #82530)

[Ingo Müller]

================
@@ -1464,6 +1465,35 @@ struct TestSignatureConversionUndo
   }
 };
 
+struct TestTestOneToNSignatureConversionNoConverter
+    : public OpConversionPattern<TestOneToNSignatureConversionNoConverterOp> {
+  TestTestOneToNSignatureConversionNoConverter(const TypeConverter &converter,
+                                               MLIRContext *context)
+      : OpConversionPattern<TestOneToNSignatureConversionNoConverterOp>(
+            context),
+        converter(converter) {}
+
+  LogicalResult
+  matchAndRewrite(TestOneToNSignatureConversionNoConverterOp op,
+                  OpAdaptor adaptor,
+                  ConversionPatternRewriter &rewriter) const final {
+    Region &region = op->getRegion(0);
+    Block *entry = &region.front();
+
+    // Convert the original entry arguments.
+    auto argTys = entry->getArgumentTypes();
+    mlir::OneToNTypeMapping argMap(argTys);
----------------
ingomueller-net wrote:

This is tricky: the test here uses `mlir::OneToNTypeMapping`, which makes it looks like that is part of the problem, but I think that this is misleading. As a very first step, I'd rewrite this test to not use any of the one-to-n code. I bet we still hit the bug, provided that we have a 1:N type conversion in the type converter.

Now, the thing is, while `TypeConverter` and also `ArgConverter`, which is used in signature conversion, have some pieces that nominally support 1:N *type* and *argument* conversion, I think that there is no code (at least in the main repository) that uses it. In particular, the (main) *dialect* conversion does not support 1:N type conversion, and the `OneToNDialectConversion` rolls its own thing, so both don't use it. Consequently, I believe that the support is only nominal and actually quite incomplete.

I don't know what the best way to fix this is: Adding the check of this PR means we don't crash for this particular test but other than the unit test, there is still no code that actually uses this code path. How do we even know whether the new (non-crashing) behavior is the right one? (@chencha3: Can you share more about the context how you triggered the bug?) Ideally, the `OneToNDialectConversion` should be merged into the main conversion logic but I have tried a long time understand how that logic works and not succeeded (which is why I created the 1:N conversion as an independent pass).

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