You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@systemds.apache.org by GitBox <gi...@apache.org> on 2020/11/12 20:02:24 UTC
[GitHub] [systemds] corepointer opened a new pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
corepointer opened a new pull request #1101:
URL: https://github.com/apache/systemds/pull/1101
Need to recompile on Windows before this is ready to merge.
Please test if that improvement has positive impact on your experiments @phaniarnab
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523476161
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -215,14 +203,14 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_sconv2dBiasAddDe
if(inputPtr == NULL || biasPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = sconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, true, (int) numThreads);
-
- return (jint) nnz;
+ size_t nnz = sconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, true, (int) numThreads);
+
+ return static_cast<jlong>(nnz);
}
-JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dBackwardDataDense(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dBackwardDataDense(
JNIEnv* env, jclass, jdoubleArray filter, jdoubleArray dout,
jdoubleArray ret, jint N, jint C, jint H, jint W, jint K, jint R, jint S,
jint stride_h, jint stride_w, jint pad_h, jint pad_w, jint P, jint Q, jint numThreads) {
Review comment:
As noted in the commit message: cleanup is WIP. I believe I gave it a two tab line continuation indent now.
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475464
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
+ setNumThreadsForBLAS(jmaxThreads);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
JNIEnv* env, jclass cls, jdoubleArray m1, jdoubleArray m2, jdoubleArray ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* m2Ptr = GET_DOUBLE_ARRAY(env, m2, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
Review comment:
nope - that's a signed integer
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475631
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
+ setNumThreadsForBLAS(jmaxThreads);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
JNIEnv* env, jclass cls, jdoubleArray m1, jdoubleArray m2, jdoubleArray ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* m2Ptr = GET_DOUBLE_ARRAY(env, m2, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
dmatmult(m1Ptr, m2Ptr, retPtr, (int)m1rlen, (int)m1clen, (int)m2clen, (int)numThreads);
+ size_t nnz = computeNNZ<double>(retPtr, m1rlen * m2clen);
RELEASE_INPUT_ARRAY(env, m1, m1Ptr, numThreads);
RELEASE_INPUT_ARRAY(env, m2, m2Ptr, numThreads);
RELEASE_ARRAY(env, ret, retPtr, numThreads);
- return (jboolean) true;
+
+ return static_cast<jlong>(nnz);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_smmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_smmdd(
JNIEnv* env, jclass cls, jobject m1, jobject m2, jobject ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
float* m1Ptr = (float*) env->GetDirectBufferAddress(m1);
float* m2Ptr = (float*) env->GetDirectBufferAddress(m2);
float* retPtr = (float*) env->GetDirectBufferAddress(ret);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
smatmult(m1Ptr, m2Ptr, retPtr, (int)m1rlen, (int)m1clen, (int)m2clen, (int)numThreads);
- return (jboolean) true;
+ return static_cast<jlong>(computeNNZ<float>(retPtr, m1rlen * m2clen));
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_tsmm
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_tsmm
(JNIEnv * env, jclass cls, jdoubleArray m1, jdoubleArray ret, jint m1rlen, jint m1clen, jboolean leftTrans, jint numThreads) {
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
Review comment:
jboolean is char, jlong is a singed integer (as is -1). Furthermore I did not get a warning by any of the used compilers (MSVC and GCC)
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475753
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -170,17 +158,17 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dDense(
if(inputPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = dconv2dBiasAddDense(inputPtr, 0, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, false, (int) numThreads);
+ size_t nnz = dconv2dBiasAddDense(inputPtr, 0, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, false, (int) numThreads);
Review comment:
Yes, that's a mess. I started to format it for a pleasant visual appearance. Of course that's not going to work out with tabs. So I changed it to two tabs for line continuation (like we have it in java).
----------------------------------------------------------------
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] [systemds] phaniarnab commented on pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
phaniarnab commented on pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#issuecomment-727284163
LGTM.
Thanks for the fix. Pushing nnz counting to native might not produce much performance in mini-batch scenario, probably due to small sizes (openmp is not useful for smaller data), but can be beneficial for higher data sizes.
----------------------------------------------------------------
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] [systemds] phaniarnab closed pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
phaniarnab closed pull request #1101:
URL: https://github.com/apache/systemds/pull/1101
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475445
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
Review comment:
done
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475424
##########
File path: src/main/cpp/libmatrixmult.h
##########
@@ -17,48 +17,19 @@
* under the License.
*/
-#ifndef _libmatrixmult_h
-#define _libmatrixmult_h
+#ifndef LIBMATRIXMULT_H
+#define LIBMATRIXMULT_H
Review comment:
Depending on your conventions - yes. I was/is a convention for most people I've worked with. I must have adopted it at some 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] [systemds] corepointer commented on pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#issuecomment-727281582
>
>
> LGTM, i have mainly commented on syntax.
Thank you for the review!
As I already mentioned in the detailed comment replies, cleanup is WIP. But this PR doesn't make more mess than has already been there. So I think it can be merged nevertheless.
----------------------------------------------------------------
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] [systemds] Baunsgaard commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523401825
##########
File path: src/main/cpp/libmatrixmult.cpp
##########
@@ -18,70 +18,42 @@
*/
#include "libmatrixmult.h"
-#include "omp.h"
-#include <cmath>
-#include <cstdlib>
-#ifdef USE_OPEN_BLAS
-#include <cblas.h>
-#else
-#include <mkl_service.h>
-#endif
+#include "common.h"
-static int SYSDS_CURRENT_NUM_THREADS = -1;
-void setNumThreadsForBLAS(int numThreads) {
- if (SYSDS_CURRENT_NUM_THREADS != numThreads) {
-#ifdef USE_OPEN_BLAS
- openblas_set_num_threads(numThreads);
-#else
- mkl_set_num_threads(numThreads);
-#endif
- SYSDS_CURRENT_NUM_THREADS = numThreads;
- }
-}
-
-void dmatmult(double *m1Ptr, double *m2Ptr, double *retPtr, int m, int k, int n,
- int numThreads) {
+void dmatmult(double *m1Ptr, double *m2Ptr, double *retPtr, int m, int k, int n, int numThreads) {
// BLAS routine dispatch according to input dimension sizes (we don't use
// cblas_dgemv with CblasColMajor for matrix-vector because it was generally
// slower than dgemm)
setNumThreadsForBLAS(numThreads);
if (m == 1 && n == 1) // VV
- retPtr[0] = cblas_ddot(k, m1Ptr, 1, m2Ptr, 1);
+ retPtr[0] = cblas_ddot(k, m1Ptr, 1, m2Ptr, 1);
Review comment:
Something went wrong with indentation in this file.
##########
File path: src/main/cpp/libmatrixmult.h
##########
@@ -17,48 +17,19 @@
* under the License.
*/
-#ifndef _libmatrixmult_h
-#define _libmatrixmult_h
+#ifndef LIBMATRIXMULT_H
+#define LIBMATRIXMULT_H
Review comment:
is there a convention for this, all CAPS?
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -170,17 +158,17 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dDense(
if(inputPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = dconv2dBiasAddDense(inputPtr, 0, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, false, (int) numThreads);
+ size_t nnz = dconv2dBiasAddDense(inputPtr, 0, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, false, (int) numThreads);
Review comment:
Indentation broken
##########
File path: src/main/cpp/systemds.h
##########
@@ -29,65 +29,65 @@ extern "C" {
/*
* Class: org_apache_sysds_utils_NativeHelper
* Method: dmmdd
- * Signature: ([D[D[DIIII)Z
+ * Signature: ([D[D[DIIII)I
Review comment:
this docs seems wierd.
does the coding of this make a difference for the docs constructed?
otherwise i would consider using double* and so on.
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
+ setNumThreadsForBLAS(jmaxThreads);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
JNIEnv* env, jclass cls, jdoubleArray m1, jdoubleArray m2, jdoubleArray ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* m2Ptr = GET_DOUBLE_ARRAY(env, m2, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
Review comment:
don't you need to cast to jlong here as well
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
+ setNumThreadsForBLAS(jmaxThreads);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_dmmdd(
JNIEnv* env, jclass cls, jdoubleArray m1, jdoubleArray m2, jdoubleArray ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* m2Ptr = GET_DOUBLE_ARRAY(env, m2, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
dmatmult(m1Ptr, m2Ptr, retPtr, (int)m1rlen, (int)m1clen, (int)m2clen, (int)numThreads);
+ size_t nnz = computeNNZ<double>(retPtr, m1rlen * m2clen);
RELEASE_INPUT_ARRAY(env, m1, m1Ptr, numThreads);
RELEASE_INPUT_ARRAY(env, m2, m2Ptr, numThreads);
RELEASE_ARRAY(env, ret, retPtr, numThreads);
- return (jboolean) true;
+
+ return static_cast<jlong>(nnz);
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_smmdd(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_smmdd(
JNIEnv* env, jclass cls, jobject m1, jobject m2, jobject ret,
jint m1rlen, jint m1clen, jint m2clen, jint numThreads)
{
float* m1Ptr = (float*) env->GetDirectBufferAddress(m1);
float* m2Ptr = (float*) env->GetDirectBufferAddress(m2);
float* retPtr = (float*) env->GetDirectBufferAddress(ret);
if(m1Ptr == NULL || m2Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
smatmult(m1Ptr, m2Ptr, retPtr, (int)m1rlen, (int)m1clen, (int)m2clen, (int)numThreads);
- return (jboolean) true;
+ return static_cast<jlong>(computeNNZ<float>(retPtr, m1rlen * m2clen));
}
-JNIEXPORT jboolean JNICALL Java_org_apache_sysds_utils_NativeHelper_tsmm
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_tsmm
(JNIEnv * env, jclass cls, jdoubleArray m1, jdoubleArray ret, jint m1rlen, jint m1clen, jboolean leftTrans, jint numThreads) {
double* m1Ptr = GET_DOUBLE_ARRAY(env, m1, numThreads);
double* retPtr = GET_DOUBLE_ARRAY(env, ret, numThreads);
if(m1Ptr == NULL || retPtr == NULL)
- return (jboolean) false;
+ return -1;
Review comment:
cast here as well
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -68,57 +51,62 @@
// -------------------------------------------------------------------
-int maxThreads = -1;
+//int maxThreads = -1;
JNIEXPORT void JNICALL Java_org_apache_sysds_utils_NativeHelper_setMaxNumThreads
(JNIEnv *, jclass, jint jmaxThreads) {
- maxThreads = (int) jmaxThreads;
+// maxThreads = (int) jmaxThreads;
Review comment:
i would remove these commented lines
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -192,18 +180,18 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_dconv2dBiasAddDe
if(inputPtr == NULL || biasPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = dconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, true, (int) numThreads);
+ size_t nnz = dconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, true, (int) numThreads);
Review comment:
indent
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -215,14 +203,14 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_sconv2dBiasAddDe
if(inputPtr == NULL || biasPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = sconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, true, (int) numThreads);
-
- return (jint) nnz;
+ size_t nnz = sconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, true, (int) numThreads);
+
+ return static_cast<jlong>(nnz);
}
-JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dBackwardDataDense(
+JNIEXPORT jlong JNICALL Java_org_apache_sysds_utils_NativeHelper_conv2dBackwardDataDense(
JNIEnv* env, jclass, jdoubleArray filter, jdoubleArray dout,
jdoubleArray ret, jint N, jint C, jint H, jint W, jint K, jint R, jint S,
jint stride_h, jint stride_w, jint pad_h, jint pad_w, jint P, jint Q, jint numThreads) {
Review comment:
Indent
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523476098
##########
File path: src/main/cpp/systemds.cpp
##########
@@ -192,18 +180,18 @@ JNIEXPORT jint JNICALL Java_org_apache_sysds_utils_NativeHelper_dconv2dBiasAddDe
if(inputPtr == NULL || biasPtr == NULL || filterPtr == NULL || retPtr == NULL)
return (jint) -1;
- int nnz = dconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr,
- (int) N, (int) C, (int) H, (int) W, (int) K, (int) R, (int) S,
- (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P, (int) Q, true, (int) numThreads);
+ size_t nnz = dconv2dBiasAddDense(inputPtr, biasPtr, filterPtr, retPtr, (int) N, (int) C, (int) H, (int) W, (int) K,
+ (int) R, (int) S, (int) stride_h, (int) stride_w, (int) pad_h, (int) pad_w, (int) P,
+ (int) Q, true, (int) numThreads);
Review comment:
Mental note: make a clang-tidy config to aid proper formatting. A plethora of editors and IDEs are in use (Visual Studio, Eclipse, IntelliJ, CLion, QT Creator, Kate, Vim, Gedit, Notepad++ all had a go at these files 😨 )
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523476513
##########
File path: src/main/cpp/systemds.h
##########
@@ -29,65 +29,65 @@ extern "C" {
/*
* Class: org_apache_sysds_utils_NativeHelper
* Method: dmmdd
- * Signature: ([D[D[DIIII)Z
+ * Signature: ([D[D[DIIII)I
Review comment:
That is machine generated stuff by javah. I couldn't resist to update the signature after changing the function's parameters even though it says specifically to not edit this file. So yes, I admit that I did not bother to run through javah again for the changes I made 😊
----------------------------------------------------------------
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] [systemds] corepointer commented on a change in pull request #1101: [SYSTEMDS-2725] NNZ counting for native blas
Posted by GitBox <gi...@apache.org>.
corepointer commented on a change in pull request #1101:
URL: https://github.com/apache/systemds/pull/1101#discussion_r523475240
##########
File path: src/main/cpp/libmatrixmult.cpp
##########
@@ -18,70 +18,42 @@
*/
#include "libmatrixmult.h"
-#include "omp.h"
-#include <cmath>
-#include <cstdlib>
-#ifdef USE_OPEN_BLAS
-#include <cblas.h>
-#else
-#include <mkl_service.h>
-#endif
+#include "common.h"
-static int SYSDS_CURRENT_NUM_THREADS = -1;
-void setNumThreadsForBLAS(int numThreads) {
- if (SYSDS_CURRENT_NUM_THREADS != numThreads) {
-#ifdef USE_OPEN_BLAS
- openblas_set_num_threads(numThreads);
-#else
- mkl_set_num_threads(numThreads);
-#endif
- SYSDS_CURRENT_NUM_THREADS = numThreads;
- }
-}
-
-void dmatmult(double *m1Ptr, double *m2Ptr, double *retPtr, int m, int k, int n,
- int numThreads) {
+void dmatmult(double *m1Ptr, double *m2Ptr, double *retPtr, int m, int k, int n, int numThreads) {
// BLAS routine dispatch according to input dimension sizes (we don't use
// cblas_dgemv with CblasColMajor for matrix-vector because it was generally
// slower than dgemm)
setNumThreadsForBLAS(numThreads);
if (m == 1 && n == 1) // VV
- retPtr[0] = cblas_ddot(k, m1Ptr, 1, m2Ptr, 1);
+ retPtr[0] = cblas_ddot(k, m1Ptr, 1, m2Ptr, 1);
Review comment:
Indentation in the native code is generally broken. I did a little cleanup here and there but that's not the focus of this PR. I refrain from further cleanup here to keep the PR somewhat readable :P
----------------------------------------------------------------
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