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

[Feature] Separate graph rewriting and constant folding #9

Open
daquexian opened this issue Sep 25, 2020 · 2 comments
Open

[Feature] Separate graph rewriting and constant folding #9

daquexian opened this issue Sep 25, 2020 · 2 comments

Comments

@daquexian
Copy link
Member

daquexian commented Sep 25, 2020

For op fusion (like the fusion of conv and bn), we have implemented a "small onnxruntime" in tensor.h. It increases the workload (the more fusion we want to do, the more op we need to implement), and brings many problems (#6, #8, onnx/onnx#2677). However, as we know, onnx itself is not designed to infer onnx ops. It is unwise to take the effort to maintain an "embedded runtime" in the presence of onnxruntime.

In my opinion, we should drop the "embedded runtime". Instead, we should only rewrite the graph, and then call onnxruntime library to fold the constants. In this way, we will not need tensor.h or another tensor library in optimizer anymore.

For example, to fuse Add(Add(x, 1), 2), instead of calculating the result of Add(1, 2) in onnx-optimizer itself, we can just rewrite the graph to Add(x, Add(1, 2)), and call onnxruntime to fold Add(1, 2) to 3.

It is also the way of tensorflow built-in optimizer.

@daquexian
Copy link
Member Author

@fumihwh @linkerzhang @askhade What's your opinion on it? Thanks!

@linkerzhang
Copy link
Member

I agree. That means for optimization like "constant folding" should not be part of ONNX repos.

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

No branches or pull requests

2 participants