-
Notifications
You must be signed in to change notification settings - Fork 38
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
Chassis platforms cleanup #1156
base: main
Are you sure you want to change the base?
Conversation
…ghtly should produce the same results
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 think we should make the default the c
platform instead of herbie10
but otherwise this looks good to merge.
The merge I think will be a little ugly, sadly. @bksaiki I say we merge this; basically this PR removes the no-pareto and no-platform options, replacing them with unusual platforms |
src/core/alt-table.rkt
Outdated
pcontext | ||
#f)) | ||
|
||
(if (> max-alts-per-pareto-point (* (*pareto-pick-limit*) 5)) |
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 am not sure that we want to leave it. This piece of code is necessary for herbie10
platform - otherwise it will be slow.
We can make something smarter, but the key point here is that pareto point will store many alternatives per point because they are all of equal cost + accuracy a lot of time is equal as well.
Therefore, the worst case is that we are storing 256*(number-of-all-the-alts-we-have-obtained)
.
I am not sure whether we are worried about herbie10
to let it merge or to make it smarter
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.
Is it still slow even with the speed-ups to pruning in main
?
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, I think that it is another problem at pruning
but only when using herbie10
though.
The speed-ups on main
help with the situation when we have just a lot of pareto-points
in general, right, we do not deduplicate them in a weird way.
But with herbie10
platform there is another problem - we have only one pareto-point
per actual point - and this pareto-point
just keeps adding almost all the alternatives it see in the same point by union-ing them. So, we end up having 256 pareto-points
, but the thing is the every pareto-point
stores a lot of alternatives in itself.
So, I keep track of it and once I catch that ~20 alternatives are already stored per point - I do atab-prune
- which cleans them up.
I was thinking that we do this cleanup dynamically without calling atab-prune
, but I have not figured out the solution
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 guess what I'm missing is—why is it bad that a pareto-point stores so many alternatives? On main
we don't do anything with these alternatives except store all of them and then call atab-prune
later.
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 just test it, on main with herbie10, and see if it's still slow.
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.
Looks like still a problem: timeline
This PR introduces the following updates:
herbie10
platform with costs 0 for all the operations/literals,herbie20
platform that is currently isdefault
of main,libm
platform to be a new default platform,libm
platform with corresponding costs,no-pareto
mode, because it should matchherbie10
platform at this point,*egraph-platform-cost*
since not used anymore,