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 2021/05/20 10:16:29 UTC
[GitHub] [tvm] Johnson9009 opened a new pull request #8090: [WIP] Just want use official CI to verify idea.
Johnson9009 opened a new pull request #8090:
URL: https://github.com/apache/tvm/pull/8090
Just want use official CI to verify idea.
--
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] [tvm] tqchen commented on pull request #8090: [WIP] Just want use official CI to verify idea.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8090:
URL: https://github.com/apache/tvm/pull/8090#issuecomment-845098489
Please avoid using CI for just verifications purposes, since CI is a resource shared by the community for code contribution gatting, since we always recommend testing things locally before sending the PR and write unit-test cases for the related change.
I can see the proposed set of changes, the particular logic of mul mod merging can likely be removed in most settings where the offset are constant(as in your case), because canonical simplifier covers that.
It is a bit unclear whether we can safely remove in settings when shape are symbolic. Constructing unit-test cases would be a good way to explore the problem
Thank you!
--
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] [tvm] tqchen commented on pull request #8090: [WIP] Just want use official CI to verify idea.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #8090:
URL: https://github.com/apache/tvm/pull/8090#issuecomment-845945503
I see, please feel free to send a PR that delete this logic and see if simplifer can handle the case. Thanks @Johnson9009
--
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] [tvm] tqchen closed pull request #8090: [WIP] Just want use official CI to verify idea.
Posted by GitBox <gi...@apache.org>.
tqchen closed pull request #8090:
URL: https://github.com/apache/tvm/pull/8090
--
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] [tvm] Johnson9009 commented on pull request #8090: [WIP] Just want use official CI to verify idea.
Posted by GitBox <gi...@apache.org>.
Johnson9009 commented on pull request #8090:
URL: https://github.com/apache/tvm/pull/8090#issuecomment-845192858
Sorry for occupation of CI resources, this idea actually come from a real bug of "ElemOffset", the current mul mod merging logic and simplifier prefer to simplify the most inner expression first, so #8093 will happen. After I know how to fix it, I strange whether we still need the code of mul mod merging, because as you said know the simplifiers of analyzer is very strong, so I have a doubt that we need the code of mul mod merging because we haven't the strong simplifier at that time.
Finally I simple delete the code of mul mod merging and want use official CI to see which case will fail, sorry for that because our internal CI haven't be set up.
--
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