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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.
Herald added a reviewer: herhut.

In D74584#1875881 <https://reviews.llvm.org/D74584#1875881>, @ftynse wrote:

> 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.


TypeConverter is a bit different than ConversionTarget with how converters get registered. The result types are mapped as:

- success: The conversion was successful
- failure: The conversion failed, abort the conversion process.
- llvm::None: The conversion failed, but the TypeConverter is allowed to fallback to another converter.

There can be different conversions that operate within the same subset of types. For example, consider the following:

  TypeConverter converter;
  
  // A converter to handle TensorTypes.
  converter.addConversion([](TensorType type) { ... });
  
  // A converter to handle a subset of types that match 'somePredicate'.
  converter.addConversion([](Type type) {
    if (somePredicate(type)) {
      ...
      return success();
    }
  
    // Returning failure means that no other converters can attempt to convert this type.
    return failure();
  
    // Returning llvm::None means that the TypeConverter can fallback to other converters.
    return llvm::None; 
  });

Without being able to toggle when fallbacks are allowed, you could only effectively have one converter registered at a time. In the above, only the 'somePredicate' converter would ever fire. What I intended for llvm::None was to differentiate "This type is illegal/isn't supported" and "I can't handle this type, but another converter might be able to." Does that make sense? If it does, is there a way that you see to make this more clear?

> 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?

I would be +1 for that. I can still see use cases for when you know for a fact the type is convertible and want a convenient API, we could likely have that constraint spelled out in the method name and assert internally that the conversion succeeds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74584