[PATCH] D74584: [mlir] Refactor TypeConverter to add conversions without inheritance

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

Thanks River! I had prototyped a similar thing, but you've beaten me to it and added fancy templates :)

I don't fully understand the meaning of return values on the conversion functions, and how they relate to the return value of `TypeConverter::convertType`.

- When we have `Optional<LogicalResult>`, what is the difference between `llvm::None` and `Failure`?
- `TypeConverter::convertType` currently returns `nullptr` on error, and it looks like it will keep doing so with both `llvm::None` and `Failure` making them indistinguishable.

I can see  use case for having `Optional<Type>`: `Optional<null-type>` means we want to erase the type, equivalent to returning an empty vector in the vector version, and `llvm::None` means there was an error, but arguably one should just use the vector version for 1->0 type conversion (the Type version would only support 1-1 mappings).

On a separate but relate subject (not for this commit), I have been repeatedly asking people to check the result of type conversion before using the type because it may fail and crash their code. Maybe we should replace `Type TypeConverter::convertType(Type)` with `LogicalResult TypeConverter::convertType(Type, Type&)` to enforce that at the API level. WDYT?



================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:177
+      if (Optional<Type> resultOpt = callback(type)) {
+        if (resultOpt && resultOpt.getValue())
+          results.push_back(resultOpt.getValue());
----------------
checking `resultOp` for being non-null is redundant here, the `if` the previous line ensured that


================
Comment at: mlir/include/mlir/Transforms/DialectConversion.h:179
+          results.push_back(resultOpt.getValue());
+        return Optional<LogicalResult>(success(resultOpt.hasValue()));
+      }
----------------
Same here, resultOpt always has value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74584