-
Notifications
You must be signed in to change notification settings - Fork 119
perf: spot neutral elements in numeric expressions #5706
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this during desugaring as opposed to the peephole optimizations we do in instrList.ml? That way we'd probably also catch things like x += 0, ....
What about other identities/constant exprs? Is there a line we want to draw somewhere?
x << 0 = x
x >> 0 = x
x | 0 = x
x | x = x
x & 0 = 0
x & x = x
x ^ x = 0
...
|
I am totally aware that we'll have to draw a line somewhere, short of including an algebraic solver/simplifier we can stop anywhere. This PR is not done yet by far. I am looking into a strange problem (bug?) where |
|
Got it 👍 Sorry for jumping the gun. |
No worries, feedback is always welcome! The right spot probably is to have a more powerful optimiser as an IR-simplifier pass, but we can do that later. The peephole optimiser is less suited for spotting |
cc632bd to
051db9e
Compare
| shift | ||
|
|
||
| if grep -q "^//SKIP $ext$" $(basename $file); then return 1; fi | ||
| local FILTER_LINE=$(grep -E "^//FILTER $ext [A-Za-z0-9 -]*$" $(basename $file) | cut -d' ' -f3-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what are these changes doing, and do we need to keep them?
| | S.LitE l -> I.LitE (lit !l) | ||
| | S.UnE (ot, o, e) -> | ||
| I.PrimE (I.UnPrim (!ot, o), [exp e]) | ||
| | S.BinE (_ot, e1, op, e2) when neutral (Either.Left op) e1 -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seems a bit strange to do this in desugaring rather than a separe IR pass, but I guess one could easily move this logic to another pass in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but out also an IR pass will burden everything with complete traversal over the full tree, while this way is very selective. But yes, we can move this out if we want some more comprehensive solution.
| //SKIP run | ||
| //SKIP run-ir | ||
| //SKIP run-low | ||
| //FILTER comp tail -n 27 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess its this.
crusso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should remove the run.sh hacks. It's complicated enough as is.
The problem is that only the tail of the IR dump is relevant for testing, and the front part is huge and if we always compare that too the brittleness of changes (e.g. prelude additions) will surface. That's why I added this shell filtering. I considered |
Fixes #5555.
TODO
or(|) — DONE: a26f4b3andcould be supported too, is it worth it?