fix overspecializing constants in compilation #22816
Conversation
eellison
changed the title
| @@ -96,6 +96,10 @@ struct ConstantTableValue : public SugaredValue { | |||
| << constants_.size() << " entries)."; | |||
| } | |||
| Value* value = m.graph()->insertConstant(constants_[offset], nullptr, loc); | |||
|
|
|||
| // specializing tensor type on compilation messes up typing relations | |||
There was a problem hiding this comment.
What typing relations does it mess up? Shouldn't we fix where those types are checked instead of throwing away the shape info?
There was a problem hiding this comment.
The shape info gets re-specialized again later anyway. It messes up things like in the example, where a list of complete tensor types does not subtype a list of tensor types, and you get error messages like:
aten::cat(Tensor[] tensors, int dim=<default>) -> Tensor:
Expected a value of type 'List[Tensor]' for argument 'tensors' but instead found type 'List[Tensor]'.
There was a problem hiding this comment.
This doesn't seem like the right place to fix this. Afterall, this same bug will exist anywhere insertConstant is used. insertConstant would be better, with shape propagation introducing a shape for a constant tensor.
There was a problem hiding this comment.
i don't think there's another pathway to create a specialized tensor type during typechecking. the shape analysis prim::Constant refinement already exists.
There was a problem hiding this comment.
@eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
When we specialize the tensor type of constants in compilation it causes all sorts of problems.
Fix for #22809