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

[tir]Analysis expr complexity support and simplify pass is turned on based on condition #7469

Closed
wants to merge 1 commit into from

Conversation

FrozenGene
Copy link
Member

As #7456 mentioned, we will meed complex expr sometimes (especially on QAT model). If we don't stop to analyzing and optimize this expr, we will meet hours+ to complete it. This pr decides to add one condition to guard it. @tqchen

@tqchen
Copy link
Member

tqchen commented Feb 21, 2021

Thanks @FrozenGene I wonder if we can also use an alternative method, e.g. add counter in the simplifiers recursive calls and throw an exception when a maximum iteration number is reached

@FrozenGene
Copy link
Member Author

Thanks @FrozenGene I wonder if we can also use an alternative method, e.g. add counter in the simplifiers recursive calls and throw an exception when a maximum iteration number is reached

Thanks @tqchen 's good suggestion. If we do like this, how do we handle this exception? continue or abort or whatever. Another thing is how do we add the counter elegantly. When we recursive call in the rewrite simplifier, we in fact analyze the whole expr and then recusive calling VisitExpr. If we want to record cnt, must we add in the class ExprFunctor's VisitExpr? I think it maybe not elegant enough.

@leeexyz
Copy link
Contributor

leeexyz commented Feb 22, 2021

Hi, this is a good and simple idea to skip the expression too complex. I just wonder if there is a case that many basic arithmetic operations composited together, and many of them can be simplified. But, here using a counter to calculate the complexity of expression and skip it if greater than a const, say 256 in the PR. Is it approprate?

@FrozenGene
Copy link
Member Author

Hi, this is a good and simple idea to skip the expression too complex. I just wonder if there is a case that many basic arithmetic operations composited together, and many of them can be simplified. But, here using a counter to calculate the complexity of expression and skip it if greater than a const, say 256 in the PR. Is it approprate?

Good question. Here assumes even we could simplify many basic arithmetic operations we still give up because it will cost hours+ to compile, compared with optimization, we choose to maintain users' experience.

@leeexyz
Copy link
Contributor

leeexyz commented Feb 22, 2021

Hi, this is a good and simple idea to skip the expression too complex. I just wonder if there is a case that many basic arithmetic operations composited together, and many of them can be simplified. But, here using a counter to calculate the complexity of expression and skip it if greater than a const, say 256 in the PR. Is it appropriate?

Good question. Here assumes even we could simplify many basic arithmetic operations we still give up because it will cost hours+ to compile, compared with optimization, we choose to maintain users' experience.

Thanks for the clarity, user experience is always the first choice. But, I think it does not consume much time compare to symbolic expressions which cannot be simplified at all (in your TFLite case).

@FrozenGene
Copy link
Member Author

Hi, this is a good and simple idea to skip the expression too complex. I just wonder if there is a case that many basic arithmetic operations composited together, and many of them can be simplified. But, here using a counter to calculate the complexity of expression and skip it if greater than a const, say 256 in the PR. Is it appropriate?

Good question. Here assumes even we could simplify many basic arithmetic operations we still give up because it will cost hours+ to compile, compared with optimization, we choose to maintain users' experience.

Thanks for the clarity, user experience is always the first choice. But, I think it does not consume much time compare to symbolic expressions which cannot be simplified at all (in your TFLite case).

Ah...unfortunately, In fact, in my tflite case, our compile time almost costs on the rewrite_simplify and canonical_simplify. If you have interest, you could try to patch #7456, remove the expression complexity restrict and run TFLite mobilenet v2 qat model (in tests/python/frontend/tflite/test_forward.py). We maybe hang hours+ to complete compling... so indeed it consume much time to compile.

@leeexyz
Copy link
Contributor

leeexyz commented Feb 24, 2021

Hi, this is a good and simple idea to skip the expression too complex. I just wonder if there is a case that many basic arithmetic operations composited together, and many of them can be simplified. But, here using a counter to calculate the complexity of expression and skip it if greater than a const, say 256 in the PR. Is it appropriate?

Good question. Here assumes even we could simplify many basic arithmetic operations we still give up because it will cost hours+ to compile, compared with optimization, we choose to maintain users' experience.

Thanks for the clarity, user experience is always the first choice. But, I think it does not consume much time compare to symbolic expressions which cannot be simplified at all (in your TFLite case).

Ah...unfortunately, In fact, in my tflite case, our compile time almost costs on the rewrite_simplify and canonical_simplify. If you have interest, you could try to patch #7456, remove the expression complexity restrict and run TFLite mobilenet v2 qat model (in tests/python/frontend/tflite/test_forward.py). We maybe hang hours+ to complete compling... so indeed it consume much time to compile.

Thanks :) Let me run it. I saw the issue in #7456 and found that the symbolic expressions are too complex to simplify.

@tqchen
Copy link
Member

tqchen commented Mar 3, 2021

Thanks @FrozenGene . I think we can just catch the special exception and return the original expression. We can just add the recursive counter to the rewriter analyzer side.

@jroesch
Copy link
Member

jroesch commented Jan 19, 2022

This PR appears to be out of date, please feel free to reopen it if this is not the case.

As part of the new year we are attempting to triage the project's open pull requests to ensure that code which
is ready for review and/or merging receives adequate attention.

Thanks again for your contribution, and feel free to reach out to discuss these changes.

@jroesch jroesch closed this Jan 19, 2022
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.

4 participants