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/04/27 23:10:26 UTC
[GitHub] [incubator-tvm] zhiics opened a new pull request #5457: [Fix] Add ConstantNode to IsAtomic
zhiics opened a new pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457
The problem is described here: https://discuss.tvm.ai/t/relay-pass-feature-check-fail-during-fold-constant-pass/6498
@MarisaKirisame Can you take a look and see if the change makes sense?
cc @kevinthesun
----------------------------------------------------------------
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] zhiics commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-621515297
@MarisaKirisame Sorry for the late response. Yeah, transforming it to ANF should work as well and seems like better fix. Let me try 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-tvm] zhiics edited a comment on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620886209
@MarisaKirisame Good question, I am not sure about how large it is. @kevinthesun had a failed cased. Maybe he can give a better answer.
----------------------------------------------------------------
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] MarisaKirisame commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-621884928
@zhiics the ANF pass is pretty fast, and it seems like if you sum the size of the graph of all call to ANF, the size will be smaller then the original graph. so i dont think it is bad.
----------------------------------------------------------------
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] kevinthesun commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-621505667
@MarisaKirisame The current use cases which can trigger is this issue are mainly from TensorFlow OD models. For those models, constant tensors which can be feed into fold_const pass are mostly small.
----------------------------------------------------------------
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] MarisaKirisame commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620973978
@zhiics I am not asking about the specific case here. I am asking that can they be big.
IIRC, they can be arbitarily big, and it seems wrong to copy them. Changing IsAtomic seems like sweeping the problem under the rug, and I think the best way to fix this is to introduce sharing before calling interpreter..
----------------------------------------------------------------
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] MarisaKirisame commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620885344
@zhiics how big is a constant? is it ok to copy them around, or is any tensor literal possible in there?
----------------------------------------------------------------
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] zhiics commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620886209
@MarisaKirisame Good question, I am not sure about large it is. @kevinthesun had a failed cased. Maybe he can give a better answer.
----------------------------------------------------------------
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] MarisaKirisame commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
MarisaKirisame commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620976285
I tooked a look at the discuss, and it seems like the way to fix it is to just call ANF from Fold-Constant before going into the interpreter pass. can you 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-tvm] zhiics edited a comment on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-621515297
@MarisaKirisame Sorry for the late response. Yeah, transforming it to ANF should work as well and seems like better fix. Let me try it.
BTW, this would also mean that that we may have to execute ANF multiple times for the program executing through VM because we do constant folding after ANF in vm optimization. In general, I don't think there wouldn't be any problem. Do you see any possible issue?
----------------------------------------------------------------
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] zhiics edited a comment on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
zhiics edited a comment on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-620886209
@MarisaKirisame Good question, I am not sure about how large it is. @kevinthesun had a failed case. Maybe he can give a better answer.
----------------------------------------------------------------
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] kevinthesun commented on pull request #5457: [Fix] Add ConstantNode to IsAtomic
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #5457:
URL: https://github.com/apache/incubator-tvm/pull/5457#issuecomment-621979744
Thanks @zhiics @MarisaKirisame
----------------------------------------------------------------
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