Skip to content

Change THTensor::size into a std::vector<int64_t>#9518

Closed
ezyang wants to merge 8 commits intopytorch:masterfrom
ezyang:pr/thtensor-size-vector
Closed

Change THTensor::size into a std::vector<int64_t>#9518
ezyang wants to merge 8 commits intopytorch:masterfrom
ezyang:pr/thtensor-size-vector

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Jul 18, 2018 •
edited
Loading

This patch was very carefully constructed to avoid having to modify
too many files; there are some obvious follow ups which I will
be hitting later.

  • I didn't do stride. But the change for stride should look
    very similar.     I did stride.

  • I did NOT rename the field in question, so that direct
    accesses of the form foo->size[n] keep working. I intend
    to do a codemod to fix all of these shortly.

  • Anywhere a "public" API function made use of a int64_t*
    of sizes, I opted to just finagle it out of the tensor using
    THTensor_getSizePtr rather than try to rewrite all of these
    sites to use ArrayRef. They should use ArrayRef eventually,
    but not yet.

  • _THSizeDesc got an overload that understands ArrayRef (which
    a vector size is convertible to). Eventually we should get
    rid of all of these functions (because ArrayRef is printable
    via the AT_ERROR macros), but not today.

  • I ran into something very subtle in the implementation of sizes()
    for TensorDerived: I MUST use the dim as per Tensor::dim() (which
    correctly is zero for scalars), otherwise I'll give a nonsense
    sizes(). We can fix this eventually once Scalar is turned on
    internally.

  • I added two new functions THTensor_resizeSize and THTensor_setSize.
    Maybe these are eventually worth deifying as methods in the Tensor class, but
    for now I'm keeping them out-of-line just in case.

Signed-off-by: Edward Z. Yang ezyang@fb.com

ezyang added 4 commits July 17, 2018 19:44
@ezyang
Change THTensor::size into a std::vector<int64_t>
0964ce9 
This patch was very carefully constructed to avoid having to modify
too many files; there are some obvious follow ups which I will
be hitting later.

- I didn't do stride.  But the change for stride should look
  very similar.

- I did NOT rename the field in question, so that direct
  accesses of the form foo->size[n] keep working.  I intend
  to do a codemod to fix all of these shortly.

- Anywhere a "public" API function made use of a int64_t*
  of sizes, I opted to just finagle it out of the tensor using
  THTensor_getSizePtr rather than try to rewrite all of these
  sites to use ArrayRef.  They should use ArrayRef eventually,
  but not yet.

- _THSizeDesc got an overload that understands ArrayRef (which
  a vector size is convertible to).  Eventually we should get
  rid of all of these functions (because ArrayRef is printable
  via the AT_ERROR macros), but not today.

- I ran into something very subtle in the implementation of sizes()
  for TensorDerived: I MUST use the dim as per Tensor::dim() (which
  correctly is zero for scalars), otherwise I'll give a nonsense
  sizes().  We can fix this eventually once Scalar is turned on
  internally.

- I added two new functions THTensor_resizeSize and THTensor_setSize.
  Maybe these are eventually worth deifying as methods in the Tensor class, but
  for now I'm keeping them out-of-line just in case.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Apparently dim doesn't match size ugh
56329cc 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Convert stride to std::vector
a797588 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
MAGMAAAA
798b7e8 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang ezyang force-pushed the pr/thtensor-size-vector branch from cce0747 to 798b7e8 Compare July 18, 2018 02:44
facebook-github-bot
facebook-github-bot reviewed Jul 18, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

gchanan
gchanan reviewed Jul 18, 2018
View reviewed changes
aten/src/TH/THGeneral.cpp Outdated
}

TH_API THDescBuff _THSizeDesc(const int64_t *size, const int64_t ndim) {
THDescBuff _THSizeDesc(at::ArrayRef<int64_t> size, const int64_t ndim) {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/TH/THGeneral.h.in Outdated
#ifdef __cplusplus
// Mangled so we can have an overload
AT_API THDescBuff _THSizeDesc(const int64_t *size, const int64_t ndim);
AT_API THDescBuff _THSizeDesc(at::ArrayRef<int64_t> size, const int64_t ndim);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

aten/src/THC/generic/THCTensor.cpp Outdated

newSize = (int64_t*)THAlloc(sizeof(int64_t)*(self->dim()+1));
newStride = (int64_t*)THAlloc(sizeof(int64_t)*(self->dim()+1));
std::vector<int64_t> newSize(/* size */ self->dim() + 1);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

gchanan
gchanan reviewed Jul 18, 2018
View reviewed changes
aten/src/THC/generic/THCTensor.cpp Outdated
THLongStorage *inferred_size = THLongStorage_newInferSize(size, numel);
auto stride = THTensor_compute_stride(at::IntList(tensor->size, tensor->dim()),
at::IntList(tensor->stride, tensor->dim()),
auto stride = THTensor_compute_stride(at::IntList(THTensor_getSizePtr(tensor), tensor->dim()),

This comment was marked as off-topic.

Sign in to view

ezyang added 4 commits July 18, 2018 08:03
@ezyang
Get rid of THSizeDesc overload.
275fa90 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
bugfix
c7bcd65 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
ci
f885f8a 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Grab some more dim_ increment/decrement
d8ca8a8 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Copy link
Contributor Author

ezyang commented Jul 18, 2018

Indeed, the inconsistency was because of some code that was doing tensor->dim_--

facebook-github-bot
facebook-github-bot reviewed Jul 18, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang mentioned this pull request Jul 18, 2018
Closed
@ezyang
Copy link
Contributor Author

ezyang commented Jul 18, 2018

Abandoning this PR in favor of #9561

@ezyang ezyang closed this Jul 18, 2018
facebook-github-bot pushed a commit that referenced this pull request Jul 19, 2018
@ezyang @facebook-github-bot
Eliminate direct access to size/strides of THTensor; replace them wit…
a08119a 
…h std::vector (#9561)

Summary:
* THTensor now stores `sizes_` and `strides_` which is a `std::vector<int64_t>`
* Anywhere a "public" API function made use of a int64_t* of sizes, I opted to just finagle it out of the tensor using THTensor_getSizePtr rather than try to rewrite all of these sites to use ArrayRef. They should use ArrayRef eventually, but not yet.
* There are new utility functions for resizing sizes/strides in one go (THTensor_resizeDim), or replacing sizes and strides with completely new values (THTensor_setSizesAndStrides)
* Anywhere you said `t->size[n] = 0`, we now say `THTensor_setSizeAt(t, n, 0)`, ditto for strides
* Anywhere you said `t->size[n]`, we now say `t->size(n)` (coming soon: ditto for strides)

Previous review of just the `std::vector` change in #9518, but I'm planning to merge this all in one go.

Note for gchanan: review from commit "ci" and after
Pull Request resolved: #9561

Reviewed By: cpuhrsch

Differential Revision: D8901926

Pulled By: ezyang

fbshipit-source-id: 483cf275060ab0a13845cba1ece39dd127142510
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@ezyang @jramseyer
Eliminate direct access to size/strides of THTensor; replace them wit…
377cca8 
…h std::vector (pytorch#9561)

Summary:
* THTensor now stores `sizes_` and `strides_` which is a `std::vector<int64_t>`
* Anywhere a "public" API function made use of a int64_t* of sizes, I opted to just finagle it out of the tensor using THTensor_getSizePtr rather than try to rewrite all of these sites to use ArrayRef. They should use ArrayRef eventually, but not yet.
* There are new utility functions for resizing sizes/strides in one go (THTensor_resizeDim), or replacing sizes and strides with completely new values (THTensor_setSizesAndStrides)
* Anywhere you said `t->size[n] = 0`, we now say `THTensor_setSizeAt(t, n, 0)`, ditto for strides
* Anywhere you said `t->size[n]`, we now say `t->size(n)` (coming soon: ditto for strides)

Previous review of just the `std::vector` change in pytorch#9518, but I'm planning to merge this all in one go.

Note for gchanan: review from commit "ci" and after
Pull Request resolved: pytorch#9561

Reviewed By: cpuhrsch

Differential Revision: D8901926

Pulled By: ezyang

fbshipit-source-id: 483cf275060ab0a13845cba1ece39dd127142510
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@ezyang
Eliminate direct access to size/strides of THTensor; replace them wit…
9c3b152 
…h std::vector (pytorch#9561)

Summary:
* THTensor now stores `sizes_` and `strides_` which is a `std::vector<int64_t>`
* Anywhere a "public" API function made use of a int64_t* of sizes, I opted to just finagle it out of the tensor using THTensor_getSizePtr rather than try to rewrite all of these sites to use ArrayRef. They should use ArrayRef eventually, but not yet.
* There are new utility functions for resizing sizes/strides in one go (THTensor_resizeDim), or replacing sizes and strides with completely new values (THTensor_setSizesAndStrides)
* Anywhere you said `t->size[n] = 0`, we now say `THTensor_setSizeAt(t, n, 0)`, ditto for strides
* Anywhere you said `t->size[n]`, we now say `t->size(n)` (coming soon: ditto for strides)

Previous review of just the `std::vector` change in pytorch#9518, but I'm planning to merge this all in one go.

Note for gchanan: review from commit "ci" and after
Pull Request resolved: pytorch#9561

Reviewed By: cpuhrsch

Differential Revision: D8901926

Pulled By: ezyang

fbshipit-source-id: 483cf275060ab0a13845cba1ece39dd127142510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchanan gchanan gchanan left review comments

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@apaszke apaszke Awaiting requested review from apaszke

@colesbury colesbury Awaiting requested review from colesbury

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ezyang @gchanan @facebook-github-bot