-
Notifications
You must be signed in to change notification settings - Fork 24.3k
Introduce type variables to implement generic list operators #12040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4814620
to
4b01b67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not a complete review, since it's WIP)
if(it == type_map.end()) { | ||
if(text.size() > 0 && islower(text[0])) { | ||
// lower case identifiers that are not otherwise valid types | ||
// are treated as type variables |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
: Type(TypeKind::ListType), elem(std::move(elem)) {} | ||
: Type(TypeKind::ListType) | ||
, elem(std::move(elem)) | ||
, has_free_variables_(getElementType()->hasFreeVariables()) {} |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zdevito has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but two commented out lines in the compiler look fishy.
return StringType::get(); | ||
} else if (kind.find("TypeVar:") == 0) { | ||
return VarType::create(kind.substr(strlen("TypeVar:"))); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/csrc/jit/script/compiler.cpp
Outdated
@@ -616,7 +630,10 @@ at::optional<std::vector<Value*>> tryMatchSchema( | |||
return at::nullopt; | |||
} | |||
} | |||
return positional_inputs; | |||
auto return_type = fmap(schema.returns, [&](const Argument& r) { |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/csrc/jit/script/compiler.cpp
Outdated
@@ -791,7 +808,7 @@ struct to_ir { | |||
// Record the type for the schema and set the Type on the Value* | |||
arguments.push_back(schema.arguments.at(arg_annotation_idx++)); | |||
new_input->setType(arguments.back().type); | |||
ensureLegalType((*it).ident().range(), arguments.back().type); | |||
// ensureLegalType((*it).ident().range(), arguments.back().type); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/csrc/jit/type.h
Outdated
}; | ||
|
||
TORCH_API TypePtr matchTypeVariables(TypePtr formal, TypePtr actual, std::unordered_map<std::string, TypePtr>& type_env); | ||
TORCH_API TypePtr evalTypeVariables(TypePtr type, std::unordered_map<std::string, TypePtr>& type_env); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
torch/csrc/jit/type.cpp
Outdated
AT_ERROR("unhandled free variable container: ", formal->str()); | ||
} | ||
|
||
// change return types like List[List[int]] into List[List[int]] |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
4b01b67
to
afa5b6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zdevito has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
afa5b6f
to
60d99f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zdevito has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: We generate specialized list operations for int, float, and Tensor lists so that small lists of integers like the arguments to conv do not involve tons of boxing code. This PR adds a fallback GenericList for List types that contain any other type. It does so by adding type variables to `jit::Type`, and machinery for matching/replacing the type variables during `tryMatchSchema` and operator lookup. It also modifies the builtin list ops to include a fallback that works on a GenericList object that simply holds IValues. This is distinguished from IValue's tuple type so that conversion to/from Python still happens losslessly. Pull Request resolved: pytorch/pytorch#12040 Differential Revision: D10037098 Pulled By: zdevito fbshipit-source-id: 0c5f2864d12e7d33554bf34cc29e5fb700dde150
We generate specialized list operations for int, float, and Tensor lists so that small lists of integers like the arguments to conv do not involve tons of boxing code.
This PR adds a fallback GenericList for List types that contain any other type. It does so by adding type variables to
jit::Type
, and machinery for matching/replacing the type variables duringtryMatchSchema
and operator lookup.It also modifies the builtin list ops to include a fallback that works on a GenericList object that simply holds IValues. This is distinguished from IValue's tuple type so that conversion to/from Python still happens losslessly.