Remove virtual keyword from most functions in TensorImpl #20909
Conversation
pytorchbot
added
the
module: internals
|
benchmark? |
gchanan
left a comment
gchanan
left a comment
There was a problem hiding this comment.
there are a bunch of comments in TensorImpl.h about avoiding virtual dispatch for perf that aren't relevant anymore.
c10/core/TensorImpl.cpp
Outdated
| TensorImpl* TensorImpl::maybe_zero_dim(bool condition_when_zero_dim) { | ||
| // We only allow this operation on dense tensors | ||
| if (!(has_storage() && layout() == kStrided)) { | ||
| TORCH_CHECK(condition_when_zero_dim == (dim() == 0), |
There was a problem hiding this comment.
I know you just moved this, but I think this should be an INTERAL_CHECK; this isn't a public API.
c10/core/UndefinedTensorImpl.h
Outdated
| int64_t size(int64_t d) const override; | ||
| int64_t stride(int64_t d) const override; | ||
| int64_t dim() const override; | ||
| bool has_storage() const override; |
There was a problem hiding this comment.
why is has_storage still virtual?
There was a problem hiding this comment.
I am thinking whether the derived types of TensorImpl is responsible for telling whether it has storage. An alternative is to explicitly check whether the current tensor is sparse or MKLDNN tensor, and if so return false in has_storage().
There was a problem hiding this comment.
I changed it to explicitly check whether the current tensor is sparse or MKLDNN tensor, and if so return false in has_storage().
| bool TensorImpl::has_storage() const { | ||
| return storage_; | ||
| TORCH_CHECK(type_id_ != UndefinedTensorId(), type_id_, " does not have has_storage"); | ||
| return !is_sparse() && !is_mkldnn() && storage_; |
There was a problem hiding this comment.
isn't this just strided? Do we ever have strided tensors where "storage_" is false?
|
|
||
| TensorImpl* TensorImpl::maybe_zero_dim(bool condition_when_zero_dim) { | ||
| // We only allow this operation on dense tensors | ||
| if (!(has_storage() && layout() == kStrided)) { |
There was a problem hiding this comment.
same as before, is there a difference between has_storage and the layout being strided?
There was a problem hiding this comment.
I guess my main question is -- can we just has_storage -> storage_ != NULL as an equivalence for kStrided? I don't know if that works with C2 tensors or not, though.
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
5 participants
As part of the Variable/Tensor merge, we want to de-virtualize TensorImpl as much as possible, to achieve performance gain for common Tensor functions such as
numel()/sizes()/dim().This supersedes #11259.