You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/07/26 17:12:48 UTC

[GitHub] [incubator-tvm] quic-sanirudh opened a new pull request #6138: Add `init` member to ReduceNode

quic-sanirudh opened a new pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138


   - This patch adds a new member to ReduceNode called init which allows
     initialization with a custom ProducerLoad or a Float/Int immediate.
   - This allows initialization of the output Tensor of a reduction with
     another Tensor instead of the `identity_element` defined in the
     CommReducer
   - One example use case for this node is to initialize the Output of a
     convolution reduction with the Bias values thereby saving the
     Bias-add computation.
   
   Thanks for contributing to TVM!   Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
   


----------------------------------------------------------------
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-tvm] tqchen commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671987519


   Oh, i meant test cases that use this feature to compile a reduction with init value like those in `tests/python/integration`


----------------------------------------------------------------
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-tvm] tqchen edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-670992782


   Thanks @quic-sanirudh for proposing the new change. My only conern wrt to the custom initialization value is that it might break the follow up primitives, e.g. rfactor and cross thread allreduce will require the init value to be the identity element. As a result, we might want to pause a bit.
   
   The initial value can still be added by introducing an additional stage(with a small overhead).
   
   There is early plan to introduce scheduling for TIR, which might bring the possibility to include such custom initialization stage, which we can then support this feature


----------------------------------------------------------------
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-tvm] quic-sanirudh commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-681232652


   I added the support for initializing with rfactor, but doesn't work with crossthread_allreduce. I also added a few unit and compiled tests.


----------------------------------------------------------------
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-tvm] quic-sanirudh edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671882417


   Thanks @tqchen for the suggestion. I'll work on adding the rfactor support and update the PR once its done.
   
   Also, could you explain what you meant by adding "compiled" testcases as I'm a little confused by that. Did you mean cpp tests?


----------------------------------------------------------------
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-tvm] tqchen commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-670992782


   Thanks @quic-sanirudh for proposing the new change. My only conern wrt to the custom initialization value is that it might break the follow up primitives, e.g. rfactor and cross thread allreduce will require the init value to be the identity element. As a result, we might want to pause a bit.
   
   The initial value can still be added by introducing an additional stage(with a small overhead).
   
   As part of this cycle, there is plan to introduce scheduling for TIR, which might bring the possibility to include such custom initialization stage, which we can then support this feature


----------------------------------------------------------------
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-tvm] tqchen commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671431691


   TIR level scheduling is still in an early stage so no RFC yet, will keep you updated once the RFC is out. 


----------------------------------------------------------------
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-tvm] quic-sanirudh commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671882417


   Thanks @tqchen for the suggestion. I'll work on adding the rfactor support and update the PR once its done.
   
   Could you explain what you meant by adding "compiled" testcases. Did you mean cpp tests?


----------------------------------------------------------------
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-tvm] tqchen merged pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138


   


----------------------------------------------------------------
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-tvm] quic-sanirudh edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671139917


   Hi @tqchen 
   Thanks for the comments. I was aware of the issue with rfactor primitive, and so I added a check that fails if `init` is used with the rfactor primitive. We could do something similar for the crossthread_allreduce as well. My thought process was that since we expect the user to understand what custom initialization means, they would in general be cautious of using it when running parallel reductions. 
   
   Also, I'm not aware of the TIR level scheduling that's planned, is there an RFC or PR where this is being discussed that I can read about.
   
   Thanks


----------------------------------------------------------------
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-tvm] quic-sanirudh commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-664016097


   This is my first time contributing to TVM and I have tried to follow all the instructions for Contributors from the documentation, but please let me know I could change anything.
   
   I ran clang-format on all the files, but wasn't sure why the cpplint is failing with errors, I'll try to fix them quickly.


----------------------------------------------------------------
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-tvm] quic-sanirudh edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-664016097


   This is my first time contributing to TVM and I have tried to follow all the instructions for Contributors from the documentation, but please let me know I could change anything.
   
   I ran clang-format on all the files, but looks like I confused clang-format with linting tools for cpp. I'll try and fix them ASAP.
   
   Thanks,
   Anirudh


----------------------------------------------------------------
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-tvm] tqchen edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671433629


   Thanks @quic-sanirudh what you said about rfactor makes sense. We can still support rfactor, by checking the factor indices, and only assign the init value if the factor indices equals the initial one, however, we may not be able to express the computation as a related primitive. Given that rfactor is not usually used together with the usage of init, this might be fine.
   
   It would also be great to add a few compiled testcases 


----------------------------------------------------------------
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-tvm] quic-sanirudh commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671992636


   > Oh, i meant test cases that use this feature to compile a reduction with init value like those in `tests/python/integration`
   
   Ah okay, thanks for the clarification. I'll add those too.


----------------------------------------------------------------
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-tvm] tqchen commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-681300404


   Thanks @quic-sanirudh ! this is now merged


----------------------------------------------------------------
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-tvm] tqchen commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671433629


   Thanks @quic-sanirudh what you said about rfactor makes sense. We can still support rfactor, by checking the factor indices, and only assign the init value if the factor indices equals the initial one. It would also be great to add a few compiled testcases


----------------------------------------------------------------
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-tvm] quic-sanirudh commented on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh commented on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671139917


   Hi @tqchen 
   Thanks for the comments. I was aware of the issue with rfactor primitive, and so I added a check that fails if `init` is used with the rfactor primitive. We could do something similar for the crossthread_allreduce as well. My thought process was that since we expect to user to understand what custom initialization means, they would in general be cautious of using it when running parallel reductions. 
   
   Also, I'm not aware of the TIR level scheduling that's planned, is there an RFC or PR where this is being discussed that I read about.
   
   Thanks


----------------------------------------------------------------
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-tvm] quic-sanirudh edited a comment on pull request #6138: Add `init` member to ReduceNode

Posted by GitBox <gi...@apache.org>.
quic-sanirudh edited a comment on pull request #6138:
URL: https://github.com/apache/incubator-tvm/pull/6138#issuecomment-671139917


   Hi @tqchen 
   Thanks for the comments. I was aware of the issue with rfactor primitive, and so I added a check that fails if `init` is used with the rfactor primitive. We could do something similar for the crossthread_allreduce as well. My thought process was that since we expect to user to understand what custom initialization means, they would in general be cautious of using it when running parallel reductions. 
   
   Also, I'm not aware of the TIR level scheduling that's planned, is there an RFC or PR where this is being discussed that I can read about.
   
   Thanks


----------------------------------------------------------------
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