This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Refactor TypeConverter to add conversions without inheritance
ClosedPublic

Authored by rriddle on Feb 13 2020, 5:08 PM.

Details

Summary

This revision refactors the TypeConverter class to not use inheritance to add type conversions. It instead moves to a registration based system, where conversion callbacks are added to the converter with addConversion. This method takes a conversion callback, which must be convertible to any of the following forms(where T is a class derived from Type:

  • Optional<Type> (T type)
    • This form represents a 1-1 type conversion. It should return nullptr or llvm::None to signify failure. If llvm::None is returned, the converter is allowed to try another conversion function to perform the conversion.
  • Optional<LogicalResult>(T type, SmallVectorImpl<Type> &results)
    • This form represents a 1-N type conversion. It should return failure or llvm::None to signify a failed conversion. If the new set of types is empty, the type is removed and any usages of the existing value are expected to be removed during conversion. If llvm::None is returned, the converter is allowed to try another conversion function to perform the conversion.

When attempting to convert a type, the TypeConverter walks each of the registered converters starting with the one registered most recently.

Diff Detail

Unit TestsFailed

Event Timeline

rriddle created this revision.Feb 13 2020, 5:08 PM
flaub added a subscriber: flaub.Feb 13 2020, 5:59 PM

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?

mlir/include/mlir/Transforms/DialectConversion.h
177

checking resultOp for being non-null is redundant here, the if the previous line ensured that

179

Same here, resultOpt always has value

This revision now requires changes to proceed.Feb 14 2020, 2:20 AM

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.

rriddle updated this revision to Diff 244861.Feb 16 2020, 3:04 AM
rriddle marked 3 inline comments as done.

Resolve comments.

mlir/include/mlir/Transforms/DialectConversion.h
177

Thanks! Merged two functions together and missed updating this.

ftynse accepted this revision.Feb 17 2020, 1:21 AM

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!

This revision is now accepted and ready to land.Feb 17 2020, 1:21 AM

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!

Thanks for the review Alex!

This revision was automatically updated to reflect the committed changes.