Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Well, this is maybe our fifth or sixth or something attempt. Frankly this shit goes back to like year 1 of the project with Alex's early simplifiers. Ten years. Fuck everything. Ok, here's the deal.
(sound-/ a b default)which means(/ a b)except whenbis zero, then it'sdefaultinstead. As far as I know there's no problems here.(sound-/ a b default) → (/ a b). The idea was that putting this after all rewrites meant later rewrites couldn't exploit the resulting(/ 0 0)terms. So it was OK to add them now.1 = (sound-/ 0 0 1) = (/ 0 0) = (sound-/ 0 0 2) = 2.sound-/to/we rewrite in reverse,(/ a b) → (sound-/ a b 0). This is always safe by the standard definition. It doesn't actually even need to be a separate rewriting phase.(/.f64 0 0)be favored over large terms? I don't know. Hasn't happened yet. Praying. In egglog, at least, I think this all will be safe since it segregates FP and R terms.Ok, so is that it? Is it done? No, for a few reasons. First of all, dropping the last argument unsoundly unions stuff. Still! Also I'm not even sure if that's fixable, at least in the egg encoding. But separately, it makes the results worse! A lot worse. Apparently the unsound merges nicely reduced the number of enodes, and with them gone the number of enodes has totally exploded. Or something? I'm not 100% sure.
So this PR includes a whole separate change, which I may split out if I have to, to reduce the number of nodes. Very simply, most of the nodes are added by the
fliprules. Why? Very simple. Those rules fire for every addition and subtraction; those are common. But they also create new additions and subtractions, so they self-activate. And they also add a lot of nodes at once.The obvious fix is—fire those more rarely by specifically selecting for square root terms. I'm not sure this will always be effective but it will for sure reduce the number of times these rules activate, which in term might result in more iterations run and therefore better results. We'll see.