[PATCH] D79729: [mlir] support materialization for 1-1 type conversions

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.
Herald added a project: MLIR.

(Sorry for the delay)

Looks really clean, thanks!



================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:116
 
+  /// Register a materialization function, which must be convertibe to the
+  /// following form
----------------
typo: convertibe


================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:117
+  /// Register a materialization function, which must be convertibe to the
+  /// following form
+  ///   `Optional<Value>(PatternRewriter &, T, ValueRange, Location)`,
----------------
nit: `form:`


================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:162
   /// On success, this function should populate 'result' with any new mappings.
   virtual LogicalResult convertSignatureArg(unsigned inputNo, Type type,
                                             SignatureConversion &result);
----------------
Can we remove the virtual here? That should enable removing the vtable completely.


================
Comment at: mlir/lib/Transforms/DialectConversion.cpp:319
       Value castValue = argInfo->castValue;
-      assert(argInfo->newArgSize > 1 && castValue && "expected 1->N mapping");
+      assert(argInfo->newArgSize >= 1);
+      assert(castValue);
----------------
Are these duplicated from below?


================
Comment at: mlir/lib/Transforms/DialectConversion.cpp:389
     auto replArgs = newArgs.slice(inputMap->inputNo, inputMap->size);
-    Operation *cast = typeConverter->materializeConversion(
-        rewriter, origArg.getType(), replArgs, loc);
-    assert(cast->getNumResults() == 1);
-    mapping.map(origArg, cast->getResult(0));
+    Value newArg = typeConverter
+                       ? typeConverter->materializeConversion(
----------------
nit: Can you just use an if for the initialization?

```
Value newArg;
if (typeConverter)
  newArg = ...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79729/new/

https://reviews.llvm.org/D79729