Port resize_as_ and clone from TH to Aten
#23027
Conversation
pytorchbot
added
module: cpu
VitalyFedyunin
changed the title
resize_as_ and clone from TH to Aten
There was a problem hiding this comment.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
VitalyFedyunin
changed the title
resize_as_ and clone from TH to Atenresize_as_ and clone from TH to Aten
Is it possible to replace those calls with ATen implementation? |
| return self; | ||
| } | ||
|
|
||
| Tensor& resize_as_cpu_(Tensor& self, const Tensor& the_template) { |
There was a problem hiding this comment.
I know that its not your change but im wondering why this file isn't located in native/cpu to follow the cuda example.
There was a problem hiding this comment.
native/cpu compiles 3 times for old CPU, old with AVX and AVX2. This code cannot be optimized with AVX
| { | ||
| THCTensor *tensor = THCTensor_(new)(state); | ||
| THCTensor_(resizeAs)(state, tensor, self); | ||
| at::Tensor tensor_wrap = THTensor_wrap(tensor); |
There was a problem hiding this comment.
i see that CPU version does wrapping, was it a bug that CUDA wasn't doing it?
There was a problem hiding this comment.
Probably just partial porting, as
does wrapping. They will eventually go away as we deprecate all call sites.
There was a problem hiding this comment.
I'm a bit confused what the strategy is here.
You don't just dispatch to at::resize_as in THCTensor_(resizeAs), but you call at::resize_as here.
There was a problem hiding this comment.
Let me follow-up with two more PRs, completly removing TH[C]Tensor_(resizeAs) and TH[C]Tensor_(newClone) code. I just afraid having hundreds line code update here, will be hard to review.
There was a problem hiding this comment.
ya that's totally fine, I just wanted to understand the strategy.
Yes, but it is about 100 call sites with additional wrapping. TH is already beaten up, have a mercy! |
|
Fixed failing tests. @izdeby might be interested to see. |
| } | ||
|
|
||
| Tensor& resize_as_cpu_(Tensor& self, const Tensor& the_template) { | ||
| return resize_cpu_(self, the_template.sizes()); |
There was a problem hiding this comment.
does this happen to fix #11665?
There was a problem hiding this comment.
In [1]: torch.randn(3).resize_as_(torch.ones(2, dtype=torch.uint8))
Out[1]: tensor([-0.5847, 1.3565])after porting
|
|
||
| void THTensor_(resizeAs)(THTensor *self, THTensor *src) | ||
| { | ||
| // already available in Aten as at::resize_as_() |
There was a problem hiding this comment.
why not call resize_as_ here?
There was a problem hiding this comment.
Well, because I had no reason to change this function as we going to nuke it all together as call sites goes away.
test/test_torch.py
Outdated
| y = x.clone() | ||
| if (device == 'cuda' and dt == torch.bfloat16): | ||
| self.assertRaises(RuntimeError, lambda: x.clone()) | ||
| self.assertRaises(RuntimeError, lambda: self.assertEqual(x, y)) |
There was a problem hiding this comment.
can you explain what's going on here? I presume this assert was in to check that clone threw an exception with cuda+bfloat16, but now it's throwing an exception on the comparison? That seems weird?
There was a problem hiding this comment.
This is funny one, as assert equals not yet functional for bfloat16, however because I fixed x.clone(), I had to update assertRaises piece.
There was a problem hiding this comment.
right, the first part makes sense because the assert is there so you have to update the test, so we are sure things get tested (otherwise these exceptional cases just live in the code forever and we never actually get test coverage).
But the second part doesn't make sense to me -- why does assertEqual throw?
There was a problem hiding this comment.
Because x -y throws =)
There was a problem hiding this comment.
@VitalyFedyunin 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.
@VitalyFedyunin 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.
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
VitalyFedyunin
added
better-engineering
Summary: API operators now routed to `at::native::resize_as_*_` and `at::native::clone` accordingly. Internal `THTensor_(resizeAs)`, `THCTensor_(resizeAs)`, `THTensor_(newClone)` and `THCTensor_(newClone)` remains to support older TH code. Pull Request resolved: pytorch/pytorch#23027 Differential Revision: D16362304 Pulled By: VitalyFedyunin fbshipit-source-id: 4c1e8516da685f3fdea632ff791d143f27aeebeb
|
@VitalyFedyunin merged this pull request in 401fbb0. |
API operators now routed to
at::native::resize_as_*_andat::native::cloneaccordingly.Internal
THTensor_(resizeAs),THCTensor_(resizeAs),THTensor_(newClone)andTHCTensor_(newClone)remains to support older TH code.