out-variant for torch.batch_norm_elemt #27621

vadimkantorov · 2019-10-09T18:32:08Z

Following dicussion with @ezyang in #26288

vadimkantorov · 2019-10-09T19:19:30Z

yes, compilation error, the same is I had got locally

ezyang · 2019-10-09T22:39:18Z

aten/src/ATen/native/cuda/Normalization.cu

Return a Tensor& here and I think your problem will go away. You might need to adjust your inner code to directly return output.

Is returning a reference to a local variable (tensor) safe?

I'm not sure I'm understanding your suggestion. Are you suggesting replacing Tensor as the return type of batch_norm_elemt_cuda_out and batch_norm_elemt_cuda by Tensor&?

Even if we change return type, in theory, the error should persist: Oct 09 19:39:07 /var/lib/jenkins/workspace/aten/src/ATen/native/cuda/Normalization.cu(67): error: cannot overload functions distinguished by return type alone

Is returning a reference to a local variable (tensor) safe?

No it is not. However, the convention is that _out functions return the reference for out that was passed in. It should look something like:

Tensor& foo_out(Tensor& output, const Tensor& input) { // ... return output; }

Even if we change return type, in theory, the error should persist

I'm not sure I follow. There might be other errors in your code, but your error message is consistent with this C++ code:

// Auto-generated in NativeFunctions.h Tensor& foo_out(Tensor& output, const Tensor& input); // Your implementation Tensor foo_out(Tensor& output, const Tensor& input) { ... }

This is exactly the situation a C++ compiler will complain about an overload distinguished only in return type.

By the way, you can find out exactly what signature ATen is expecting by inspecting the generated native functions header.

@ezyang Thanks for explanation! I now understand you, I'll try the suggested return type for the _out-variant.

vadimkantorov · 2019-10-12T15:38:19Z

clang-tidy check error seems unrelated

And compilation fails with:

HEAD is now at 3e7b27a2be Tensor -> Tensor& as out-variant return type
+ git merge --no-edit --no-ff fcb6dd079eb01b594aeea9f7c83ea1b36aa65793
fatal: refusing to merge unrelated histories
Exited with code 128

Do I need to rebase?

Also, https://github.com/vadimkantorov/pytorch/blob/master/aten/src/ATen/native/cuda/Normalization.cuh/#L678-L679 in an extreme case could reallocate, and then returning a reference to output_ (before reshape) is incorrect. In practice this should never happen. Is there a simple way to ensure that reshape does not reallocate?

ezyang · 2019-10-14T15:19:21Z

Do I need to rebase?

Yep.

Also, https://github.com/vadimkantorov/pytorch/blob/master/aten/src/ATen/native/cuda/Normalization.cuh/#L678-L679 in an extreme case could reallocate, and then returning a reference to output_ (before reshape) is incorrect

You should error in that case. Replace this with a view which is guaranteed not to reallcoate?

vadimkantorov · 2019-10-14T23:24:25Z

You should error in that case. Replace this with a view which is guaranteed not to reallcoate?
Ok, I'll replace the output_.reshape with output.view. On a side note, currently there is no easy way for:

a = torch.rand(2,3,4)
a_ = a.transpose(-1, -2)
b_ = someltwisefunc(a_.reshape(2, -1)) # reshape seems to reallocate (checked by data_ptr), view will error out
b = b_.view_as(a)

vadimkantorov · 2019-10-15T12:27:47Z

Squashed and rebased. I hope it's done correctly (I added remote upstream, fetched it, squashed commits and then rebased, it's my first time using git rebase, so sorry for this verbosity):

git remote add upstream https://github.com/pytorch/pytorch.git
git fetch upstream master
git rebase -i HEAD~2 # pick + squash
git rebase upstream/master
git push -f

If compilation works, I'll replace output_.reshape to output_.view

ezyang · 2019-10-15T14:05:24Z

Build failure is real


Oct 15 13:15:06 /var/lib/jenkins/workspace/aten/src/ATen/native/cuda/Normalization.cu(70): error: initial value of reference to non-const must be an lvalue
Oct 15 13:15:06

ezyang · 2019-10-15T14:21:13Z

aten/src/ATen/native/cuda/Normalization.cuh

view here, to avoid realloc

ezyang · 2019-10-15T14:21:54Z

The rest of the code looks reasonable!

vadimkantorov · 2019-10-15T14:27:19Z

Build failure is real


Oct 15 13:15:06 /var/lib/jenkins/workspace/aten/src/ATen/native/cuda/Normalization.cu(70): error: initial value of reference to non-const must be an lvalue
Oct 15 13:15:06

Is it because AT_DISPATCH_FLOATING_TYPES_AND_HALF and the lambda passed in are implicitly returning a Tensor, not Tensor&? Would you have an advice how to fix that?

vadimkantorov · 2019-10-16T08:48:31Z

aten/src/ATen/native/cuda/Normalization.cu

One way would be to remove returns here and replace them by return output at the end. Should I do that @ezyang?

Yeah, that's what I would do.

Tensor -> Tensor& as out-variant return type batch_norm_elemt_cuda_template: reshape -> view for output batch_norm_elemt_cuda_template return type: Tensor& -> void

vadimkantorov · 2019-10-16T21:53:37Z

finally it compiled :)

facebook-github-bot

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

Summary: Following dicussion with ezyang in pytorch/pytorch#26288 Pull Request resolved: pytorch/pytorch#27621 Differential Revision: D17978858 Pulled By: ezyang fbshipit-source-id: f843b691a67f1dc48b87ed6a633007d193150cf7

facebook-github-bot · 2019-10-17T17:37:02Z

@ezyang merged this pull request in e1be08f.

Summary: Following dicussion with ezyang in pytorch#26288 Pull Request resolved: pytorch#27621 Differential Revision: D17978858 Pulled By: ezyang fbshipit-source-id: f843b691a67f1dc48b87ed6a633007d193150cf7

pytorchbot added module: cuda Related to torch.cuda, and CUDA support in general module: operators labels Oct 9, 2019

ezyang reviewed Oct 9, 2019

View reviewed changes

vadimkantorov mentioned this pull request Oct 11, 2019

[feature request] Reduction (torch.add / torch.logaddexp / torch.max / torch.min / torch.mean) of several tensors without extra copies/allocations / memory accesses } TensorList inputs support #27522

Open

vadimkantorov mentioned this pull request Oct 14, 2019

batch_norm_elemt_cuda_template does not use its argument epsilon #27466

Closed

ezyang reviewed Oct 15, 2019

View reviewed changes

aten/src/ATen/native/cuda/Normalization.cuh Outdated

Copy link

Contributor

ezyang Oct 15, 2019

Choose a reason for hiding this comment

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

view here, to avoid realloc

vadimkantorov commented Oct 16, 2019

View reviewed changes

out-variant for torch.batch_norm_elemt

06e481e

Tensor -> Tensor& as out-variant return type batch_norm_elemt_cuda_template: reshape -> view for output batch_norm_elemt_cuda_template return type: Tensor& -> void

ezyang approved these changes Oct 17, 2019

View reviewed changes

facebook-github-bot reviewed Oct 17, 2019

View reviewed changes

facebook-github-bot closed this in e1be08f Oct 17, 2019

vadimkantorov mentioned this pull request Oct 17, 2019

Feature request: support for BatchNorm1d variant mapillary/inplace_abn#138

Closed

facebook-github-bot added the merged label Oct 17, 2019

This was referenced Oct 17, 2019

Supporting old-style classes in torch.is_tensor and torch.is_storage #1005

Closed

Inplace and out arguments for BatchNorm (and other norm layers: InstanceNorm / LayerNorm / GroupNorm ...) #26288

Open

mruberry added the Merged label Oct 28, 2020

out-variant for torch.batch_norm_elemt #27621

out-variant for torch.batch_norm_elemt #27621

Uh oh!

Conversation

vadimkantorov commented Oct 9, 2019

Uh oh!

vadimkantorov commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadimkantorov Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadimkantorov commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Oct 14, 2019

Uh oh!

vadimkantorov commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadimkantorov commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezyang commented Oct 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang commented Oct 15, 2019

Uh oh!

vadimkantorov commented Oct 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadimkantorov commented Oct 16, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vadimkantorov Oct 10, 2019 •

edited

Loading

vadimkantorov commented Oct 12, 2019 •

edited

Loading

vadimkantorov commented Oct 14, 2019 •

edited

Loading

vadimkantorov commented Oct 15, 2019 •

edited

Loading