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 2021/09/07 17:19:29 UTC
[GitHub] [systemds] Baunsgaard opened a new pull request #1385: [SYSTEMDS-???] Rewrite cbind 0 Matrix Multiplication
Baunsgaard opened a new pull request #1385:
URL: https://github.com/apache/systemds/pull/1385
This PR contains a rewrite that change the sequences if number of rows in X is 2x larger than Y:
```
cbind((X %*% Y), matrix(0, nrow(X), 1)) -> X %*% (cbind(Y, matrix(0, nrow(Y), 1)))
```
This rewrite effects MLogReg in line 215 to not force allocation of the large X twice.
Review appreciated!
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on pull request #1385: [SYSTEMDS-3123] Rewrite cbind 0 Matrix Multiplication
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1385:
URL: https://github.com/apache/systemds/pull/1385#issuecomment-915559306
no, you misunderstood my main comment - we need to compare m vs n (and apparently that's what you wanted to do), but what the code does is to compare m vs (k+1) because you look at rows and columns of the output cbind (variable hi) - simply replace `hi.getDim2()` with `hi.getInput(1).getDim1()`
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] mboehm7 commented on pull request #1385: [SYSTEMDS-3123] Rewrite cbind 0 Matrix Multiplication
Posted by GitBox <gi...@apache.org>.
mboehm7 commented on pull request #1385:
URL: https://github.com/apache/systemds/pull/1385#issuecomment-915533612
Thanks for staring the discussion, a few additional points to consider (besides separating it from the compression-related changed):
* we need to check for know dims, otherwise the rewrite also triggers if hi.getDim2() is unknown (-1), and as we don't consider the other way around, it would never come back during recompilation
* The overall benefit comes from reduced allocation in the cbind output (while incurring more floating point operations in the matrix multiply in some kernels that avoid branches in the inner loop). Accordingly, we cannot just look at `hi.getDim1() > hi.getDim2() * 2`. Instead given a m-x-n matrix X and n-x-k matrix Y, we have to compare the following: (a): m*k + m*(k+1) versus (b) n*k + m*(k+1), which reduces to comparing m and n, not m and (k+1), isn't 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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Shafaq-Siddiqi commented on pull request #1385: [SYSTEMDS-???] Rewrite cbind 0 Matrix Multiplication
Posted by GitBox <gi...@apache.org>.
Shafaq-Siddiqi commented on pull request #1385:
URL: https://github.com/apache/systemds/pull/1385#issuecomment-915100846
LGTM!
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard closed pull request #1385: [SYSTEMDS-3123] Rewrite cbind 0 Matrix Multiplication
Posted by GitBox <gi...@apache.org>.
Baunsgaard closed pull request #1385:
URL: https://github.com/apache/systemds/pull/1385
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [systemds] Baunsgaard commented on pull request #1385: [SYSTEMDS-3123] Rewrite cbind 0 Matrix Multiplication
Posted by GitBox <gi...@apache.org>.
Baunsgaard commented on pull request #1385:
URL: https://github.com/apache/systemds/pull/1385#issuecomment-915556882
> Thanks for staring the discussion, a few additional points to consider (besides separating it from the compression-related changed):
I merged in the compression changes when i was happy with the rewrite.
I will separate the different elements in different commits once it goes to master.
> * we need to check for know dims, otherwise the rewrite also triggers if hi.getDim2() is unknown (-1), and as we don't consider the other way around, it would never come back during recompilation
Yes good point, i will include it in the a check that the dims are known
> * The overall benefit comes from reduced allocation in the cbind output (while incurring more floating point operations in the matrix multiply in some kernels that avoid branches in the inner loop). Accordingly, we cannot just look at `hi.getDim1() > hi.getDim2() * 2`. Instead given a m-x-n matrix X and n-x-k matrix Y, we have to compare the following: (a): m_k + m_(k+1) versus (b) n_k + m_(k+1), which reduces to comparing m and n, not m and (k+1), isn't it?
The comparison i make currently is based on the extra allocation therefore it has to compare m (the left hand side number of rows) with n (the common dimension/ number of rows in right).
this is because these two correspond to the memory allocation in the cbind operation. To make the rewrite better i could include a cost of the matrix multiplication for cbind before vs after. this would make the rewrite more stable.
Alternatively i think what i am going to do is to set the `hi.getDim1() > hi.getDim2() * 2` to something like `hi.getDim1() > hi.getDim2() * 100`, since this still tricker the rewrite in the place intended, and is less prone to executing the rewrite in bad locations. (not that it matters much it is a very specific rewrite.)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@systemds.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org