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

[Relay][Pass] Avoid FoldConstant folding some ops #4245

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

vinx13
Copy link
Member

@vinx13 vinx13 commented Nov 1, 2019

Add a list of ops that should be constant evaluated. Current use case is fixed point multiplication (with TONEAREST rounding mode), which generate constant tensor for rounding. Ideally we expect it to an inlined scalar.

@anijain2305 @zhiics

@vinx13 vinx13 force-pushed the feature/fold_const_opt branch from 60c8508 to 310b3a3 Compare November 1, 2019 14:53
@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

@vinx13 BTW, we do expect constant folding for integer values, right?

@vinx13
Copy link
Member Author

vinx13 commented Nov 1, 2019

@zhiics Do you mean scalar values?

@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

@vinx13 I actually meant both scalar and TensorType with DataType as is_int/is_uint.

@vinx13
Copy link
Member Author

vinx13 commented Nov 1, 2019

The ops skipped all initialize tensors with scalar value. Is there any use cases we want to actually create these tensors as constant?

@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

yeah, I am not aware of such a use case. I was curious because I thought it was a rounding problem, but integer values would not have it. Could you explain a bit more why we need to keep those operators? If so, should we also keep zeros and ones?

@vinx13
Copy link
Member Author

vinx13 commented Nov 1, 2019

This is mostly performance issue. For example, full create a tensor with all elements of the same value. Folding it and then read the value of the tensor from memory at runtime is inefficient. We would like to lower the scalar value directly to topi level. For ops with zero args (e.g. zeros and ones) they are already skipped before this pr.

@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

I see. Thanks.

@zhiics zhiics merged commit aa49e85 into apache:master Nov 1, 2019
@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

Thanks @vinx13 @anijain2305 This is now merged.

@jroesch
Copy link
Member

jroesch commented Nov 1, 2019

We should make this configurable, this is good for some use cases but hardwiring this set of ops for other use cases doesn't make sense, for example in the VM it is better to manifest these as constants and load into constant pool then doing it at runtime.

@zhiics
Copy link
Member

zhiics commented Nov 1, 2019

Yeah, Jared is right. I was hesitating on this as well. @vinx13 Can you send another PR to make it configurable? Sorry for the inconvenience.

@vinx13
Copy link
Member Author

vinx13 commented Nov 1, 2019

This PR basically extends the rule in
https://github.com/vinx13/tvm/blob/e5e5b1b5b7658dfc77029c740ec8abe249f5b159/src/relay/pass/fold_constant.cc#L111-L114
Even for the vm case, I think it is still beneficial fusing to other ops instead of folding to constant

zxy844288792 pushed a commit to neo-ai/tvm that referenced this pull request Nov 13, 2019
* [Relay][Pass] Avoid FoldConstant folding some ops

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

Successfully merging this pull request may close these issues.

4 participants