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

Remove the algebraic simplifier #655

Closed
athas opened this issue Oct 23, 2018 · 2 comments
Closed

Remove the algebraic simplifier #655

athas opened this issue Oct 23, 2018 · 2 comments

Comments

@athas
Copy link
Member

athas commented Oct 23, 2018

I have done some more profiling and found that the algebraic simplifier is still a huge cost sink. Worse, it is already so restricted that it barely ever does anything. Thus, I propose that we simply remove it entirely, and replace it with a very simple pattern-based rule that can handle the few bounds checks that are currently removed by the simplifier. In the future, we can bring it back in a better and faster form (it's also wrong in exotic cases right now).

As sample motivation, disabling the algebraic simplifier cuts the compile time of LocVolCalib (with incremental flattening) from over 9 seconds to about 5 seconds. Other benchmarks are similar.

@athas
Copy link
Member Author

athas commented Oct 23, 2018

I have looked at bit at what it actually does for us at the moment. If we simply disable the algebraic simplifier, then a bunch of programs will need unsafe. The vast majority of cases are:

  • A loop or map over iota n where we are using the element to index an array of n elements.
  • A nested iota n where we must know that n is non-negative (and this is in fact guaranteed by some outer loops).

It should not be hard to create a much simpler and more efficient replacement that can handle just these cases.

@athas
Copy link
Member Author

athas commented Oct 25, 2018

I did not remove the algebraic simplifier, but I moved its usage into a separate pass that is only run once. The improvement in compiler speed is significant. The simplifier itself will remain for now, but I still do not like it.

@athas athas closed this as completed Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant