You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/08/14 05:02:31 UTC

[GitHub] [incubator-mxnet] MoisesHer opened a new pull request #18921: Einsum cutensor GPU

MoisesHer opened a new pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921


   ## Description ##
   Implementation of einsum on GPU though [cuTensor](https://developer.nvidia.com/cutensor) library
   
   ## Checklist ##
   ### Essentials ###
   - [ ] Changes are complete (i.e. I finished coding on this PR)
   - [x] All changes have test coverage:
   - Unit tests are added for different einsum scenarios
   - [x] Code is well-documented: 
   - [x] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
   
   ### Changes ###
   - [x] Added a new implementation of einsum operator though cuTensor library
   
   ## Comments ##
   TODO:
   - Include Broadcast cases
   - Include Identity cases
   - Include single operator with cuTensor
   - Infer right part of equation
   - Doubts about operator signature: same signature for FWD & BWD?
   - Cache vs. Signature approaches (alignment at descriptors creation)
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471827712



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       No that doesn't really make a difference, we have to consider them either way.
   
   We now again have competing operator implementations which are chosen at compile time - numpyGpu vs cutensor. This creates another situation where from a user perspective, we are adding another magic knob which improves performance but is pretty hidden. As a product, we should make mxnet smart enough to figure these things out at runtime - run with cutensor if available and fall back to numpy implementation (our own) if not available. But adding even more compile time branching sounds totally user-unfriendly in my opinion. 
   
   In the end, this might totally worth for a benchmark scenario or a situation where somebody knows exactly what's happening in every detail of mxnet, but the standard user should get a good Performance without having to know every single detail of every single operator and which build flags to choose - this is a more fundamental thing. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-674447518


   Why make this optional at all? Why not just add it and we're fine? This just adds test complexity otherwise


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sxjscience commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
sxjscience commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-703034002


   I find that there are some lint errors.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471601166



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       I'm a bit concerned that we add too many things which depend on optional dependencies that are determined at compile time. This make releasing mxnet more and more difficult. Can we fine a neater way? Like either saying that cutensor is now a required dependency if cudnn and/or cuda are enabled or put them into the external operator space that is currently being developed? We are creating more and more branching within our build system that is impossible to test properly. 
   
   Another topic is of course the whole licensing topic. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471824816



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       there shouldn't be a licensing concern given that it's an optional dependency




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-673892604


   @MoisesHer we deprecated Makefile usage for master. could you add that logic to cmake instead?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-698697226


   Jenkins CI successfully triggered : [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471824816



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       there shouldn't be a licensing concern given that it's an optional dependency
   
   UPDATE:
   if it wasn't clear, I'm addressing this comment specifically:
   > Another topic is of course the whole licensing topic.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-698697226


   Jenkins CI successfully triggered : [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer edited a comment on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer edited a comment on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-699198571


   @springer13 we are having difficulties to identify what combinations are covered or not by cuTensor.
   Can you please take a look at this function and help to define a correct an exclusion criteria? [https://github.com/apache/incubator-mxnet/pull/18921/files#diff-97b34ff5dce0b1e4346953b5ca2b5bc5R471](https://github.com/apache/incubator-mxnet/pull/18921/files#diff-97b34ff5dce0b1e4346953b5ca2b5bc5R471)
   
   Please, follow numpy-einsum convention


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sxjscience commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r495110765



##########
File path: 3rdparty/mshadow/mshadow/stream_gpu-inl.h
##########
@@ -196,10 +206,75 @@ struct Stream<gpu> {
     CHECK_EQ(err, CUDNN_STATUS_SUCCESS) << cudnnGetErrorString(err);
 #endif
   }
+#if MSHADOW_USE_CUTENSOR == 1
+  inline static cutensorHandle_t GetCuTensorHandle(Stream<gpu> *stream) {
+    //if (stream == NULL) {
+    //  return 0;
+    //} else {

Review comment:
       Should we check that *stream cannot be NULL?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-698697206


   @mxnet-bot run ci [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-694614949


   There are quite a bit of cpp lint errors. https://jenkins.mxnet-ci.amazon-ml.com/blue/organizations/jenkins/mxnet-validation%2Fsanity/detail/PR-18921/6/pipeline#step-80-log-514


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479698491



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       having intelligent runtime loading would certainly be nice. This doesn't mean that we should force Moises to work on this, and I think whoever works on it should provide an RFC to help provide clarity on how it would work eventually.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479507046



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       Not all, but we have to start somewhere. We can't make every feature a compile time option and then say that it's because it's generally not possible. We should start small and incrementally get to that state.
   
   Just this feature as example. We would need an entire different build plus a few full test suite runs to test this particular flavour. And then when talk about publishing, we either have to decide if we enable or disable it - the other option will then get totally forgotten because not many people will bother to compile with every single flag changed. 
   
   If we do it at runtime, we could have systems รก la autotuning which determine the best flags (or allows the user to easily toggle), but if we don't start somewhere, we will never get to that point. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471590622



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       Thanks, will add. But will keep as OFF by default.
   cutensor is not part of cuda 11, so user needs to download independently: [cutensor](https://developer.nvidia.com/cutensor)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-699198571


   @springer13 we are having difficulties to identify what combinations are covered or not by cuTensor.
   Can you please take a look at this function? [https://github.com/apache/incubator-mxnet/pull/18921/files#diff-97b34ff5dce0b1e4346953b5ca2b5bc5R471](https://github.com/apache/incubator-mxnet/pull/18921/files#diff-97b34ff5dce0b1e4346953b5ca2b5bc5R471)
   
   Please, follow numpy-einsum convention


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479455775



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       While I agree that we should have good defaults for build options, I don't think we should mix any of the build options together. We can't foresee how downstream users want to use certain features, and easy customization is essential in accommodating as much of the customized usage as possible. Many successful projects have a large number of build options for this reason: https://github.com/opencv/opencv/blob/master/CMakeLists.txt




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-704396662


   > @springer13 we are having difficulties to identify what combinations are covered or not by cuTensor.
   > Can you please take a look at this function and help to define a correct an exclusion criteria? https://github.com/apache/incubator-mxnet/pull/18921/files#diff-97b34ff5dce0b1e4346953b5ca2b5bc5R471
   > 
   > Please, follow numpy-einsum convention
   
   This has been solved


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479508094



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       I know that the c++ world tends towards compile time options because it's just so easy with all the preprocessor statements, but smart loading (e.g. only when the appropriate dependencies and environment are present) and runtime feature toggles are something that can even be done in the c++ world. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479500460



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       I agree that users should be able to choose, but I'm arguing that we should rather do that at run time opposed to having every nitty gritty option at compile time.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-698697206


   @mxnet-bot run ci [unix-cpu]


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] MoisesHer commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
MoisesHer commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-699204248


   > LGTM. One minor comment about `stream == NULL`.
   
   I removed that function, since we were not using it


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sxjscience merged pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
sxjscience merged pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r470769299



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       Please add the option at the mxnet/CMakeLists.txt, analogous to USE_CUDNN. I guess it should default to ON? Is cutensor included by default in cuda 11?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r471827712



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       No that doesn't really make a difference, we have to consider them either way.
   
   We now again have competing operator implementations which are chosen at compile time - numpyGpu vs cutensor. This creates another situation where from a user perspective, we are adding another magic knob which improves performance but is pretty hidden. As a product, we should make mxnet smart enough to figure these things out at runtime - run with cutensor if available and fall back to numpy implementation (our own) if not available. But adding even more compile time branching sounds totally user-unfriendly in my opinion. 
   
   In the end, this might totally work for a benchmark scenario or a situation where somebody knows exactly what's happening in every detail of mxnet, but the standard user should get a good Performance without having to know every single detail of every single operator and which build flags to choose - this is a more fundamental thing. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sxjscience commented on pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
sxjscience commented on pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#issuecomment-688031960


   For me, I think we may enable cutensor by default given that it has better performance. The problem is I'm not sure if it's possible to do that.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] sxjscience commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
sxjscience commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r495110765



##########
File path: 3rdparty/mshadow/mshadow/stream_gpu-inl.h
##########
@@ -196,10 +206,75 @@ struct Stream<gpu> {
     CHECK_EQ(err, CUDNN_STATUS_SUCCESS) << cudnnGetErrorString(err);
 #endif
   }
+#if MSHADOW_USE_CUTENSOR == 1
+  inline static cutensorHandle_t GetCuTensorHandle(Stream<gpu> *stream) {
+    //if (stream == NULL) {
+    //  return 0;
+    //} else {

Review comment:
       Should we check that stream cannot be NULL?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479700879



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       Sure




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-mxnet] szha commented on a change in pull request #18921: Einsum cutensor GPU

Posted by GitBox <gi...@apache.org>.
szha commented on a change in pull request #18921:
URL: https://github.com/apache/incubator-mxnet/pull/18921#discussion_r479503958



##########
File path: 3rdparty/mshadow/CMakeLists.txt
##########
@@ -59,6 +59,9 @@ endif()
 if(USE_CUDNN)
   target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUDNN)
 endif()
+if(USE_CUTENSOR)
+  target_compile_definitions(mshadow INTERFACE MSHADOW_USE_CUTENSOR)
+endif()

Review comment:
       having the option to select features at runtime means that all features must be built into the binary, which is based on the assumption that such build is feasible.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org