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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

In D74584#1878076 <https://reviews.llvm.org/D74584#1878076>, @rriddle wrote:

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


The documentation exactly as in the list above should be sufficient. I could also interpret the negative results in the opposite way: `llvm::None` - error happened, abort the process; `failure()` - failed to convert with the callback, try the next one, so I'd prefer stated explicitly. The only thing that is more clear is a special tri-state return type with `Success`, `TryNext`, `Abort`, but it's probably an overkill.

I don't immediately see the case where one would want to disallow other converters, but they may appear given the infra allows it.

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

Makes sense to me!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74584