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

River Riddle via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Feb 17 01:34:07 PST 2020


rriddle added a comment.

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

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


SGTM, will leave as-is for now and add a tri-state later if the readability becomes an issue.

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

The current cases I've seen are for example; I want to reuse the LLVMTypeConverter but override the conversion for a specific type(e.g. VectorType). There is a possibility that we can solve this in a better way, but I'll leave that for a followup when we have more concrete usages.

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





More information about the llvm-commits mailing list