CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules #35916

Zjq9409 · 2021-09-22T08:45:53Z

PR types

Others

PR changes

OPs

Describe

Eigh算子前向计算将eigen库替换为Lapack库实现，修改linalg暴露规则。

paddle-bot-old · 2021-09-22T08:46:01Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Xreki · 2021-09-24T06:39:07Z

paddle/fluid/operators/math/eigen_values_vectors.h

-      input_tensor = dito.Transpose(input);
+        math::DeviceIndependenceTensorOperations<platform::CPUDeviceContext, T>(
+            ctx);
+    *eigen_vectors = dito.Transpose(input);


lapack输入是列优先的？最好加一些注释说明，这里为什么要transpose。

看了下svd_helper.h里面实现的Transpose，既然只针对最好2维进行transpose，其实没必要区分2-6 D，eigvals_op.h里面SpiltBatchSquareMatrix这种实现方式比较好。

EigvalsKernel里面，math::lapackEig<T, Real<T>>这种调用方式也比较好，能够清楚的体现values、vectors的数据类型的关系。

如果has_vectors为false，好像是否转置得到的特征值都是一样的，所以这里可能不需要transpose？

Xreki · 2021-09-24T06:44:18Z

paddle/fluid/operators/math/eigen_values_vectors.h

@@ -128,37 +50,74 @@ struct MatrixEighFunctor<platform::CPUDeviceContext, ValueType, T> {
  void operator()(const framework::ExecutionContext &ctx, const Tensor &input,


functor里面不再需要从ctx里面获取运行时信息，最好传DeviceContext。

Xreki · 2021-09-24T06:45:36Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    int liwork = -1;
+    int iwork_opt = -1;
+    T lwork_opt = static_cast<T>(-1);
+    ValueType rwork_opt = static_cast<ValueType>(-1);


这些参数都是啥意思？-1代表什么？lapack函数中会写这些参数吗？可以加一些注释说明下。

Xreki · 2021-09-24T07:27:04Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    auto *infos_data = info_tensor.mutable_data<int>(
+        framework::make_ddim({batch_size}), ctx.GetPlace());
+
+    math::lapackEvd<T, ValueType>(jobz, uplo, n, out_vector, lda, out_value,


加个注释说明单独这一次lapack函数调用的目的是什么。

Xreki · 2021-09-24T07:38:50Z

paddle/fluid/operators/math/eigen_values_vectors.h

+              *info_ptr));
+    }
+    if (has_vectors) {
+      *eigen_vectors = dito.Transpose(*eigen_vectors);


确认下，前后2个Transpose是必须的吗？

Xreki · 2021-09-24T07:39:57Z

paddle/fluid/operators/math/eigen_values_vectors.h

+      auto *value_data = out_value + i * values_stride;
+      auto *vector_data = out_vector + i * vector_stride;
+      int *info_ptr = &infos_data[i];
+      math::lapackEvd<T, ValueType>(jobz, uplo, n, vector_data, lda, value_data,


Evd是什么意思，会被用到别的计算吗，是否直接叫lapackEigh比较好？

Xreki · 2021-09-24T07:42:05Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    T lwork_opt = static_cast<T>(-1);
+    ValueType rwork_opt = static_cast<ValueType>(-1);
+
+    Tensor info_tensor;


如果一次只计算1个矩阵，貌似不用定义一个info_tensor，而是直接用int info就够？

Xreki · 2021-09-24T07:42:57Z

paddle/fluid/operators/math/eigen_values_vectors.h

+      PADDLE_ENFORCE_EQ(
+          *info_ptr, 0,
+          platform::errors::PreconditionNotMet(
+              "For batch [%d]: the [%d] argument had an illegal value", i,


这个报错信息不太具有实际含义。

Xreki · 2021-09-24T07:46:05Z

paddle/fluid/operators/math/lapack_function.cc

+}
+
+template <>
+void lapackEvd<paddle::platform::complex<float>, float>(


这里可不必加paddle::

Xreki · 2021-09-24T12:45:29Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    // lapack is a column-first storage, transpose make the eigen_vectors to
+    // have a continuous memory layout
+    *eigen_vectors = dito.Transpose(input);
+    auto *out_vector = eigen_vectors->mutable_data<T>(ctx.GetPlace());


has_vectors为false时，eigen_vectors为nullptr，不应该调mutable_data。

Xreki · 2021-09-24T12:46:10Z

paddle/fluid/operators/math/eigen_values_vectors.h

-      input_tensor = dito.Transpose(input);
+        math::DeviceIndependenceTensorOperations<platform::CPUDeviceContext, T>(
+            ctx);
+    // lapack is a column-first storage, transpose make the eigen_vectors to


一般是说column-major storge。

Xreki · 2021-09-24T12:47:04Z

paddle/fluid/operators/math/eigen_values_vectors.h

-      input_tensor = dito.Transpose(input);
+        math::DeviceIndependenceTensorOperations<platform::CPUDeviceContext, T>(
+            ctx);
+    *eigen_vectors = dito.Transpose(input);


如果has_vectors为false，好像是否转置得到的特征值都是一样的，所以这里可能不需要transpose？

Xreki · 2021-09-24T12:47:54Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    // Call lapackEigh to get the optimal size of work data
+    math::lapackEigh<T, math::Real<T>>(jobz, uplo, n, out_vector, lda,
+                                       out_value, &lwork_opt, lwork, &rwork_opt,
+                                       lrwork, &iwork_opt, liwork, &info);


这里不需要检查info吗？

Xreki · 2021-09-24T12:49:15Z

paddle/fluid/operators/math/eigen_values_vectors.h

+    int iwork_opt = -1;  // The optimal length of the array liwork
+    T lwork_opt = static_cast<T>(-1);  // The optimal length of the array work
+    ValueType rwork_opt =
+        static_cast<ValueType>(-1);  // The optimal length of the array rwork


可以using ValueType = math::Real<T>;，这样模板里面不用传ValueType。

Xreki

LGTM

TCChenlong

LGTM

…sure rules (PaddlePaddle#35916)

) (#36091) cherry-pick #35916，CPU前向计算将Eigen替换为Lapack，修改linalg暴露规则

…sure rules (PaddlePaddle#35916)

CPU forward calculation replaces Eigen with Lapack

9e4cf0d

Remove memory copy operation

8d53c2f

Zjq9409 force-pushed the LapackEigh branch from f1f21d9 to 8d53c2f Compare September 23, 2021 07:10

Zjq9409 added 3 commits September 23, 2021 08:30

Merge branch 'develop' into LapackEigh

7aa8183

Merge branch 'develop' into LapackEigh

dbadce0

Modify format

8733853

Zjq9409 changed the title ~~CPU forward calculation replaces Eigen with Lapack~~ CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules Sep 24, 2021

Merge branch 'develop' into LapackEigh

10b7ce8

Xreki reviewed Sep 24, 2021

View reviewed changes

Modify the incoming type parameters, add comments

7b8aa10

Zjq9409 force-pushed the LapackEigh branch 2 times, most recently from d4df8ec to a0c1b49 Compare September 24, 2021 10:51

Zjq9409 force-pushed the LapackEigh branch from a0c1b49 to 0181ac3 Compare September 24, 2021 12:30

Xreki reviewed Sep 24, 2021

View reviewed changes

Merge branch 'develop' into LapackEigh

1c0fe59

Zjq9409 force-pushed the LapackEigh branch from 0181ac3 to 1c0fe59 Compare September 25, 2021 12:36

Support eigenvectors nullptr

3cf4e85

Zjq9409 force-pushed the LapackEigh branch from a6c34c4 to 3cf4e85 Compare September 26, 2021 02:20

Xreki approved these changes Sep 26, 2021

View reviewed changes

TCChenlong approved these changes Sep 26, 2021

View reviewed changes

Xreki merged commit 7ff226f into PaddlePaddle:develop Sep 26, 2021

Zjq9409 added a commit to Zjq9409/Paddle that referenced this pull request Sep 26, 2021

CPU forward calculation replaces Eigen with Lapack;Modify linalg expo…

4c4b315

…sure rules (PaddlePaddle#35916)

Zjq9409 mentioned this pull request Sep 26, 2021

[cherry-pick]CPU forward calculation replaces Eigen with Lapack(#35916) #36091

Merged

lanxianghit pushed a commit that referenced this pull request Sep 26, 2021

[cherry-pick]CPU forward calculation replaces Eigen with Lapack (#35916

effb70f

) (#36091) cherry-pick #35916，CPU前向计算将Eigen替换为Lapack，修改linalg暴露规则

AnnaTrainingG pushed a commit to AnnaTrainingG/Paddle that referenced this pull request Sep 29, 2021

CPU forward calculation replaces Eigen with Lapack;Modify linalg expo…

af0dd25

…sure rules (PaddlePaddle#35916)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules #35916

CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules #35916

Zjq9409 commented Sep 22, 2021 •

edited

Loading

paddle-bot-old bot commented Sep 22, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki Sep 24, 2021

Xreki left a comment

TCChenlong left a comment

		@@ -128,37 +50,74 @@ struct MatrixEighFunctor<platform::CPUDeviceContext, ValueType, T> {
		void operator()(const framework::ExecutionContext &ctx, const Tensor &input,

CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules #35916

CPU forward calculation replaces Eigen with Lapack;Modify linalg exposure rules #35916

Conversation

Zjq9409 commented Sep 22, 2021 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented Sep 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xreki left a comment

Choose a reason for hiding this comment

TCChenlong left a comment

Choose a reason for hiding this comment

Zjq9409 commented Sep 22, 2021 •

edited

Loading