Enforce same input tensor storage in VariableType functions #16305

yf225 · 2019-01-24T05:48:44Z

In VariableType.cpp, when a function modifies its input tensors, it should only change the input tensors' storage data in-place, and should never change the input tensors' storage pointers. This PR adds checks for this, and also fixes functions that fail this test.

This is part of the Variable/Tensor merge work (#13638).

gchanan · 2019-01-24T16:19:30Z

aten/src/ATen/native/BatchLinearAlgebra.cpp

-  std::tie(solution, lu) = at::_gesv_helper(self, A);
+  Tensor solution_tmp, lu_tmp;
+  std::tie(solution_tmp, lu_tmp) = at::_gesv_helper(self, A);
+  solution.resize_as_(solution_tmp);


nit: it's a little nicer to read if you do this all on one line, e.g.:
solution.resize_as_(solution_tmp).copy_(solution_tmp);

gchanan · 2019-01-24T16:21:12Z

tools/autograd/gen_variable_type.py

    '_coalesced_',
 }

+# When a function modifies its input tensors, it should only change the input tensors'


this isn't quite accurate -- we usually call resize, which means we are only guaranteed to keep the storage pointer if the size is equal (or less? is that right? can you check?).

Resizing a tensor shows the following behavior:

Expanding a tensor: always creates a new data_ptr, doesn't create a new StorageImpl or a new Storage wrapper.

Shrinking a tensor: doesn't create a new data_ptr / StorageImpl / Storage.

For our purpose I think we should only check that StorageImpl stays the same after the internal function call (by checking storage.is_alias_of(storage_original), and we should allow the internal function to resize the tensor / change the data_ptr as needed.

gchanan · 2019-01-24T16:24:03Z

tools/autograd/gen_variable_type.py

 baseType->${method_prefix_derived}${base_name}(${unpacked_args})""")

+SAVE_TENSOR_STORAGE_PTR = CodeTemplate("""\
+StorageImpl* ${tensor_name}_storage_ptr_saved = (${tensor_name}.defined() && !${tensor_name}.is_sparse()) ? ${tensor_name}.storage().unsafeGetStorageImpl() : nullptr;


why do we need to look at the actual StorageImpl and not just the data pointer? Also, can't the Storage go away from under us because we aren't holding onto a reference?

If we only checks the tensor data_ptr, we won't be able to allow in-place resize in the internal function (since in-place expanding a tensor will change its data_ptr), which might be putting too much of a constraint on what the internal function can do. Checking Storage or StorageImpl avoids this constraint, while still ensuring that data is being put into the original Storage.

gchanan · 2019-01-24T16:26:37Z

tools/autograd/gen_variable_type.py

+            if dynamic_type == 'TensorList':
+                unpacked_tensorlists.append(arg['name'])
+            elif dynamic_type != 'SparseTensorRef':
+                unpacked_tensors.append(arg['name'])


how do we know these are actually tensors?

requires_unpack(arg) above checks whether the arg's dynamic_type contains Tensor, and the if 'TensorOptions' not in dynamic_type: check eliminates the TensorOptions case, so we know that these args are guaranteed to be tensors.

This reverts commit 0ba8a3a.

This reverts commit d102d2c.

yf225 · 2019-01-31T02:40:19Z

I created an issue #16589 to track the leftover work for enforcing data_ptr equality for input tensors that have non-zero size.

facebook-github-bot

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

tools/autograd/gen_variable_type.py

gchanan · 2019-02-01T21:16:59Z

tools/autograd/gen_variable_type.py

+# The following list contains functions that we don't enforce the invariant on.
+DONT_ENFORCE_SAME_TENSOR_IMPL_OR_STORAGE = {
+    # These functions are expected to change impl or storage of input tensors
+    '_th_set_', '_cudnn_rnn_flatten_weight',


are you going to change these?

It's likely not possible to fix _th_set_ because changing storage is exactly its purpose. For fixing _cudnn_rnn_flatten_weight, I opened an issue at #16695.

tools/autograd/gen_variable_type.py

gchanan · 2019-02-01T21:45:41Z

tools/autograd/gen_variable_type.py

+std::vector<Storage> ${tensorlist_name}_storage_saved(${tensorlist_name}.size());
+for (size_t i=0; i<${tensorlist_name}.size(); i++) {
+  ${tensorlist_name}_storage_saved[i] =
+    (${tensorlist_name}[i].defined() && !${tensorlist_name}[i].is_sparse()) ?


can you use std::transform or something similar here?

I feel that using std::transform might make the code a bit harder to read. I changed this to

for (Tensor tensor : ${tensorlist_name}) ${tensorlist_name}_storage_saved.push_back(tensor.has_storage() ? tensor.storage() : Storage());

tools/autograd/gen_variable_type.py

This reverts commit 0ff71d7.

This reverts commit 357bf2e.

gchanan · 2019-02-08T23:35:17Z

tools/autograd/gen_variable_type.py

+            for arg in env.get('unpacked_args', []):
+                simple_type = env['unpacked_args_simple_type'][arg]
+                if simple_type == 'TensorList':
+                    save_ptrs_block += SAVE_STORAGE_AND_IMPL.substitute(


nit: do you actually have to build these into strings over just keeping them as lists? I thought CodeTemplates could handle those.

Fixed by using lists instead of strings :)

This reverts commit df8dc9f.

facebook-github-bot

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

Summary: In VariableType.cpp, when a function modifies its input tensors, it should only change the input tensors' storage data in-place, and should never change the input tensors' storage pointers. This PR adds checks for this, and also fixes functions that fail this test. This is part of the Variable/Tensor merge work (pytorch/pytorch#13638). Pull Request resolved: pytorch/pytorch#16305 Differential Revision: D13897855 Pulled By: yf225 fbshipit-source-id: 0c4fc7eb530d30db88037b1f0981f6f8454d3b79

…16305) Summary: In VariableType.cpp, when a function modifies its input tensors, it should only change the input tensors' storage data in-place, and should never change the input tensors' storage pointers. This PR adds checks for this, and also fixes functions that fail this test. This is part of the Variable/Tensor merge work (pytorch#13638). Pull Request resolved: pytorch#16305 Differential Revision: D13897855 Pulled By: yf225 fbshipit-source-id: 0c4fc7eb530d30db88037b1f0981f6f8454d3b79

Enforce same input tensor storage in VariableType functions

bfe6683

yf225 requested review from ezyang and gchanan January 24, 2019 05:48

yf225 mentioned this pull request Jan 24, 2019

Variable/Tensor Merge Proposal #13638

Closed

22 tasks

fix cuda bug

9a476b6

gchanan reviewed Jan 24, 2019

View reviewed changes

Will Feng and others added 18 commits January 24, 2019 11:34

improve gesv_out

2c03a0b

save Storage instead of StorageImpl pointer

2aac7ea

simplify codegen

0815c30

fix lint

600727c

fix bug

24b9766

check tensor.data_ptr() instead

0ba8a3a

DEBUG: expect gesv to fail

d102d2c

Revert "check tensor.data_ptr() instead"

7c4c68b

This reverts commit 0ba8a3a.

check impl as well

842b95b

check only in DEBUG mode

db48a3e

improve codegen

1356656

check unpacked tensors

0f12a27

fix bug

ee03a80

fix DEBUG

c972fe8

Revert "DEBUG: expect gesv to fail"

f5fa802

This reverts commit d102d2c.

Merge branch 'master' into gesv_fix_pointer_equality

2242577

fix lint

e8a0b6e

better naming

7b97e6c

yf225 mentioned this pull request Jan 31, 2019

Make sure data_ptr for non-zero-size input tensors stays the same after the VariableType dispatch #16589

Open

facebook-github-bot reviewed Jan 31, 2019

View reviewed changes

gchanan reviewed Feb 1, 2019

View reviewed changes

yf225 mentioned this pull request Feb 2, 2019

Check whether _cudnn_rnn_flatten_weight can avoid changing the TensorImpl or Storage pointer of tensors in weight_arr #16695

Open

organize gen_variable_type.py better

71f9c56

yf225 force-pushed the gesv_fix_pointer_equality branch 4 times, most recently from 0fa5946 to 2f65222 Compare February 8, 2019 21:47

Use simple_type

d1c9eef

yf225 force-pushed the gesv_fix_pointer_equality branch from 2f65222 to d1c9eef Compare February 8, 2019 21:49

Will Feng added 2 commits February 8, 2019 16:51

fix bug

8a541ed

make codegen better

8519448

yf225 force-pushed the gesv_fix_pointer_equality branch from 866c19d to 8519448 Compare February 8, 2019 22:11

Will Feng added 4 commits February 8, 2019 17:18

use Optional for TensorImpl equality test

357bf2e

This reverts commit 0ff71d7.

Revert "use Optional for TensorImpl equality test"

cbad07c

This reverts commit 357bf2e.

fix lint

0b2bd42

fix codegen

d7bcb67

yf225 force-pushed the gesv_fix_pointer_equality branch from 51438d7 to d7bcb67 Compare February 8, 2019 23:09

gchanan approved these changes Feb 8, 2019

View reviewed changes

Will Feng added 2 commits February 8, 2019 18:55

use UndefinedTensorImpl::singleton()

b067718

simplify codegen

ab34bb7

yf225 force-pushed the gesv_fix_pointer_equality branch from 46ab5a3 to ab34bb7 Compare February 11, 2019 16:16

Will Feng added 2 commits February 11, 2019 11:20

fix codegen

9064ac5

DEBUG: test NDEBUG

df8dc9f

yf225 force-pushed the gesv_fix_pointer_equality branch from f81735f to df8dc9f Compare February 11, 2019 16:25

Revert "DEBUG: test NDEBUG"

ec9309f

This reverts commit df8dc9f.

facebook-github-bot reviewed Feb 11, 2019

View reviewed changes

facebook-github-bot closed this in e2a5b20 Feb 11, 2019

ezyang added the merged label Jun 25, 2019

albanD mentioned this pull request Feb 7, 2020

Native function returning argument autograd hazard #25927

Closed

soulitzer mentioned this pull request Jun 25, 2021

Check native_function's outputs' TensorImpl and StorageImpl #60286

Closed

Enforce same input tensor storage in VariableType functions #16305

Enforce same input tensor storage in VariableType functions #16305

Uh oh!

Conversation

yf225 commented Jan 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yf225 Jan 24, 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!

yf225 Jan 24, 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!

yf225 commented Jan 31, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yf225 Jan 24, 2019 •

edited

Loading

yf225 Jan 24, 2019 •

edited

Loading