[7102][fb_bootcamp] Moved Gesv to ATen #9742

animesht · 2018-07-24T00:59:01Z

Moved _gesv_single and _gesv_single_out to ATen

aten/src/ATen/native/Gesv.cpp

+  if (!batched) {
+    Tensor A_;
+    Tensor b_;
+    return _gesv_single_helper(self, A, b_, A_);


ssnl

_gesv_single_helper doesn't work for cuda. _th_gesv_single isn't removed.

ssnl

need to remove thc gesv code too.

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

+      ALLOCATE_ARRAY(info_array, magma_int_t, 1, result0);
+      ALLOCATE_ARRAY(ipiv_array, magma_int_t*, 1, result0);
+
+      magmaGesvBatched<scalar_t>(


ssnl · 2018-07-25T13:46:29Z

test failure still looks legit

ssnl · 2018-07-26T04:51:36Z

@pytorchbot retest this please

aten/src/ATen/native/Gesv.cpp

+  AT_DISPATCH_FLOATING_TYPES(self.type(), "gesv", [&]{
+    auto A_ptr = result1.data<scalar_t>();
+    auto b_ptr = result0.data<scalar_t>();
+    auto ipiv = at::empty({bx}, result0.type().toScalarType(kInt));


aten/src/ATen/native/native_functions.yaml

    CPU: _gesv_helper_cpu
    CUDA: _gesv_helper_cuda

+- func: _gesv_single(Tensor self, Tensor A) -> (Tensor, Tensor)


animesht · 2018-07-27T03:57:54Z

@pytorchbot retest this please

ssnl

Overall looks great! Some small nits.

aten/src/ATen/native/Gesv.cpp


 std::tuple<Tensor&,Tensor&> gesv_out(
-    Tensor& solution, Tensor& lu, const Tensor& self, const Tensor& A) {
+    Tensor& result0, Tensor& result1, const Tensor& self, const Tensor& A) {


aten/src/ATen/native/native_functions.yaml

 - func: gesv(Tensor self, Tensor A) -> (Tensor, Tensor)

- func: gesv_out(Tensor solution, Tensor lu, Tensor self, Tensor A) -> (Tensor, Tensor)
+- func: gesv_out(Tensor result0, Tensor result1, Tensor self, Tensor A) -> (Tensor, Tensor)


tools/autograd/derivatives.yaml

 - name: _gesv_single(Tensor self, Tensor A)
  self: gesv_backward_self(grad, self, A)
-  A: gesv_backward_A(grad, self, A, solution)
+  A: gesv_backward_A(grad, self, A, result0)


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

  return std::tuple<Tensor,Tensor>(b_working_copy, A_working_copy);
 }

+std::tuple<Tensor&,Tensor&> _gesv_single_out_cuda(Tensor& result0, Tensor& result1,


facebook-github-bot

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

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

+  int info;
+  int* ipiv;
+
+  result1 = A.t().clone();


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

+    "compilation. Please rebuild with MAGMA.");
+#else
+  int64_t bx = self.size(0);
+  int64_t by = (self.dim() == 1) ? 1 : self.size(1);


aten/src/ATen/native/Gesv.h

+          (long long)A.size(0), (long long)A.size(1));
+    } else if (A.size(0) != self.size(0)) {
+      AT_ERROR("A,B size incompatible - A has %ld "
+          "rows, B has %ld cols", A.size(0), self.size(0));


zou3519

This looks good, @animesht! I had a few minor comments and questions, in particular:

As @ssnl mentioned, it would be nice if we could rename (result0, result1) to (solution, lu) (or whatever the ordering is) if the code generation allows for it.
~~2) Tensors sent into the magma/lapack gesv bindings are required to be in column-major format: I'm not sure if the .t().clone() that is being done is sufficient for this~~ nvm, this is fine!

aten/src/ATen/native/Gesv.cpp

+  int64_t by = (self.dim() == 1) ? 1 : self.size(1);
+  int info = 0;
+
+  result1 = A.t().clone();


facebook-github-bot

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

animesht · 2018-07-27T16:31:29Z

Renamed result0, result1
Fixed docs to mention that B can be 1D
Addressed other nits

aten/src/ATen/native/Gesv.cpp

+   *            - clone and copy input_tensor.t() to output_tensor (same tensor)
+   *         b) &input_tensor != &output_tensor:
+   *            - copy input_tensor.t() to output_tensor
+   */


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

+AT_ERROR("gesv: MAGMA library not found in "
+    "compilation. Please rebuild with MAGMA.");
+#else
+  /* See Gesv.cpp for comments */


zou3519

Haven't gone through the code in detail yet. I left some comments on refactoring the code to be a little nicer

animesht · 2018-08-17T19:03:11Z

@zou3519, addressed your comments and refactored the implementation to Gesv.h and made it more compact. Hope this looks better!

cc @ssnl

facebook-github-bot

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

yf225 · 2018-08-21T16:48:25Z

@pytorchbot retest this please

aten/src/ATen/native/Gesv.h

+ *    (i)  &in == &out: copy in.t().clone() to out (same tensor)
+ *    (ii) &in != &out: copy in.t() to out
+ */
+static inline void prepareIOTensors(


aten/src/ATen/native/Gesv.h

+ */
+static inline void prepareIOTensors(
+    const Tensor& in, Tensor& out, Tensor& temp,
+    int64_t& x, int64_t& y) {


aten/src/ATen/native/Gesv.h

+ * Before passing pointers to Lapack, we need to ensure that these pointers
+ * represent Fortran-contiguous tensors in column-major format
+ *
+ * Cases:


aten/src/ATen/native/Gesv.h

+ * (ii) for 2D matrices, .t_() represents their column-major format
+ *
+ * Before passing pointers to Lapack, we need to ensure that these pointers
+ * represent Fortran-contiguous tensors in column-major format


aten/src/ATen/native/Gesv.h

+ * copy the buffer to the output tensor.
+ *
+ * 2) out.t() is contiguous:
+ *    (i)  &in == &out: use out.data() as is. Do nothing


aten/src/ATen/native/Gesv.h

+  // view potential 1D `in` as 2D
+  auto in_t = in.view({x, y}).t_();
+
+  if (!out_tc && !out.is_contiguous() && out_correct_shape) {


aten/src/ATen/native/Gesv.cpp

+  Tensor temp_sol;
+  Tensor temp_lu;
+  prepareTensorsForLapack(self, sol, temp_sol);
+  prepareTensorsForLapack(A, lu, temp_lu);


aten/src/ATen/native/Gesv.cpp

+  prepareTensorsForLapack(A, lu, temp_lu);
+
+  AT_DISPATCH_FLOATING_TYPES(self.type(), "gesv", [&]{
+    const int64_t bx = sol.size(0);


yf225 · 2018-08-28T17:06:19Z

ping @animesht :)

animesht · 2018-08-28T22:30:02Z

Addressed most comments @zou3519 . Still need to add the tests for each case to test_torch.py, I actually had these as uncommitted python files in my old devgpu which I'm locked out of :(

Hoping for that to come back up soon, otherwise I can rewrite those and add them

animesht · 2018-09-06T00:09:30Z

@zou3519 added tests for all cases. Can I get a codeowner review?

fmassa · 2018-11-06T17:59:34Z

This got superseded by #6100

Moved Gesv to ATen

1090e20

animesht requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners July 24, 2018 00:59

animesht changed the title ~~[1702][fb_bootcamp] Moved Gesv to ATen~~ [7102][fb_bootcamp] Moved Gesv to ATen Jul 24, 2018

ssnl reviewed Jul 24, 2018

View reviewed changes

aten/src/ATen/native/Gesv.cpp Outdated

if (!batched) {

Tensor A_;

Tensor b_;

return _gesv_single_helper(self, A, b_, A_);

This comment was marked as off-topic.

Sign in to view

ssnl previously requested changes Jul 24, 2018

View reviewed changes

Removed TH code, added CUDA support

2144c29

ssnl reviewed Jul 25, 2018

View reviewed changes

aten/src/ATen/native/cuda/Gesv.cu Outdated

ALLOCATE_ARRAY(info_array, magma_int_t, 1, result0);

ALLOCATE_ARRAY(ipiv_array, magma_int_t*, 1, result0);

magmaGesvBatched<scalar_t>(

This comment was marked as off-topic.

Sign in to view

Fixed CUDA support, removed gesv from THC, fixed autograd+tests

f64b620

ssnl reviewed Jul 26, 2018

View reviewed changes

animesht added 3 commits July 26, 2018 11:05

Used options().dtype(kInt)

e9cd831

addressed comments - simplified gesv_single

333b230

Fixed autograd

152f0cb

ssnl reviewed Jul 27, 2018

View reviewed changes

facebook-github-bot reviewed Jul 27, 2018

View reviewed changes

zou3519 reviewed Jul 27, 2018

View reviewed changes

aten/src/ATen/native/cuda/Gesv.cu Outdated

int info;

int* ipiv;

result1 = A.t().clone();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Jul 27, 2018

View reviewed changes

aten/src/ATen/native/cuda/Gesv.cu Outdated

"compilation. Please rebuild with MAGMA.");

#else

int64_t bx = self.size(0);

int64_t by = (self.dim() == 1) ? 1 : self.size(1);

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Jul 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.h Outdated

(long long)A.size(0), (long long)A.size(1));

} else if (A.size(0) != self.size(0)) {

AT_ERROR("A,B size incompatible - A has %ld "

"rows, B has %ld cols", A.size(0), self.size(0));

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Jul 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.cpp Outdated

int64_t by = (self.dim() == 1) ? 1 : self.size(1);

int info = 0;

result1 = A.t().clone();

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

Addressed comments

e20d906

facebook-github-bot reviewed Jul 27, 2018

View reviewed changes

zou3519 reviewed Aug 16, 2018

View reviewed changes

aten/src/ATen/native/cuda/Gesv.cu Outdated

AT_ERROR("gesv: MAGMA library not found in "

"compilation. Please rebuild with MAGMA.");

#else

/* See Gesv.cpp for comments */

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Aug 16, 2018

View reviewed changes

Refactored

0838375

Fixed typo

f0e0739

facebook-github-bot reviewed Aug 17, 2018

View reviewed changes

animesht added 2 commits August 17, 2018 14:40

Fix indent

c1bd0a0

fix jenkins

a01c96e

zou3519 reviewed Aug 23, 2018

View reviewed changes

animesht added 4 commits August 23, 2018 11:58

fix jenkins

dac0811

Addressed comments

c980e34

Addressed comments

1bb8d22

fixed typo

5341fee

zou3519 reviewed Aug 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.h

* Before passing pointers to Lapack, we need to ensure that these pointers

* represent Fortran-contiguous tensors in column-major format

*

* Cases:

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Aug 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.h

* (ii) for 2D matrices, .t_() represents their column-major format

*

* Before passing pointers to Lapack, we need to ensure that these pointers

* represent Fortran-contiguous tensors in column-major format

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Aug 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.h

* copy the buffer to the output tensor.

*

* 2) out.t() is contiguous:

* (i) &in == &out: use out.data() as is. Do nothing

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Aug 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.cpp Outdated

Tensor temp_sol;

Tensor temp_lu;

prepareTensorsForLapack(self, sol, temp_sol);

prepareTensorsForLapack(A, lu, temp_lu);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

zou3519 reviewed Aug 27, 2018

View reviewed changes

aten/src/ATen/native/Gesv.cpp Outdated

prepareTensorsForLapack(A, lu, temp_lu);

AT_DISPATCH_FLOATING_TYPES(self.type(), "gesv", [&]{

const int64_t bx = sol.size(0);

This comment was marked as off-topic.

Sign in to view

Addressed comments

5973c4e

Added tests

6c573ec

fmassa closed this Nov 6, 2018

soumith mentioned this pull request Dec 3, 2018

Move gesv to ATen #7102

Closed

[7102][fb_bootcamp] Moved Gesv to ATen #9742

[7102][fb_bootcamp] Moved Gesv to ATen #9742

Uh oh!

Conversation

animesht commented Jul 24, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl commented Jul 25, 2018

Uh oh!

ssnl commented Jul 26, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

animesht commented Jul 27, 2018

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

zou3519 left a comment • 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!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

animesht commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

zou3519 left a comment •

edited

Loading

animesht commented Jul 27, 2018 •

edited

Loading