This is an archive of the discontinued LLVM Phabricator instance.

[MLInliner] Simplify TFUTILS_SUPPORTED_TYPES
ClosedPublic

Authored by mtrofin on Aug 25 2020, 10:00 AM.

Details

Summary

We only need the C++ type and the corresponding TF Enum. The other
parameter was used for the output spec json file, but we can just
standardize on the C++ type name there.

Diff Detail

Event Timeline

mtrofin created this revision.Aug 25 2020, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2020, 10:00 AM
mtrofin requested review of this revision.Aug 25 2020, 10:00 AM
ebrevdo accepted this revision.Aug 25 2020, 10:39 AM

This TypeSpec will no longer match specs generated by stringifying tensorflow types. For example.

In [4]: tf.int64.name 
Out[4]: 'int64'

meaning we will have to do the string conversion in python instead of in C++; either way we can't really get away from it. Your choice where you want us to do it.

This revision is now accepted and ready to land.Aug 25 2020, 10:39 AM

This is intentional. We mainly wanted to avoid a third type alias, i.e. we have LLVM's aliases (float, doble, int32_t, etc); and tensorflow's enums (TF_FLOAT, TF_DOUBLE, TF_INT32). The uses of these 2 groups is typechecked. The one we eliminated, the string, which was only used in json, was not. Arguably, the resulting code is more robust.

This TypeSpec will no longer match specs generated by stringifying tensorflow types. For example.

In [4]: tf.int64.name 
Out[4]: 'int64'

meaning we will have to do the string conversion in python instead of in C++; either way we can't really get away from it. Your choice where you want us to do it.

yundiqian accepted this revision.Aug 25 2020, 11:21 AM

Yes, Mircea and I discussed about it. Doing conversion in either place is fine. The argument of doing the it here is better isolation of TF and LLVM and easier testing in LLVM

This TypeSpec will no longer match specs generated by stringifying tensorflow types. For example.

In [4]: tf.int64.name 
Out[4]: 'int64'

meaning we will have to do the string conversion in python instead of in C++; either way we can't really get away from it. Your choice where you want us to do it.

This revision was landed with ongoing or failed builds.Aug 25 2020, 2:20 PM
This revision was automatically updated to reflect the committed changes.