Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Add ConstantNode to IsAtomic #5457

Merged
merged 2 commits into from Apr 30, 2020
Merged

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Apr 27, 2020

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

@MarisaKirisame
Copy link
Contributor

@zhiics how big is a constant? is it ok to copy them around, or is any tensor literal possible in there?

@zhiics
Copy link
Member Author

zhiics commented Apr 28, 2020

@MarisaKirisame Good question, I am not sure about how large it is. @kevinthesun had a failed case. Maybe he can give a better answer.

@MarisaKirisame
Copy link
Contributor

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

@MarisaKirisame
Copy link
Contributor

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?

@kevinthesun
Copy link
Contributor

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

@zhiics
Copy link
Member Author

zhiics commented Apr 29, 2020

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

@MarisaKirisame
Copy link
Contributor

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

Copy link
Contributor

@kevinthesun kevinthesun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinthesun kevinthesun merged commit ae89afe into apache:master Apr 30, 2020
@kevinthesun
Copy link
Contributor

Thanks @zhiics @MarisaKirisame

@zhiics zhiics deleted the fix_featuremap branch April 30, 2020 17:32
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 9, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
zhiics added a commit to zhiics/tvm that referenced this pull request Jun 16, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
zhiics added a commit to zhiics/tvm that referenced this pull request Jun 16, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
zhiics added a commit to zhiics/tvm that referenced this pull request Jun 17, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
zhiics added a commit to zhiics/tvm that referenced this pull request Jun 17, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
zhiics added a commit to zhiics/tvm that referenced this pull request Jun 17, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
tqchen pushed a commit that referenced this pull request Jun 17, 2020
* [Fix] Add ConstantNode to IsAtomic (#5457)

* add constantnode to atomic

* Add ToANormalForm to FoldConstant

* fix test
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Jun 18, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Jun 18, 2020
* add constantnode to atomic

* Add ToANormalForm to FoldConstant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants