-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
optimizer: use count checking framework #44794
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
Conversation
|
Concept LGTM. One DRY comment on the implementation. |
ec95383 to
7312685
Compare
f9686d0 to
995f2f4
Compare
|
Gonna add some basic |
6cce43c to
574291f
Compare
c53dc98 to
4e32885
Compare
|
@Keno I think this is ready now --- the interface to actually use the checking framework is a bit clunky, but I'm not sure of a better way to throw such an error from inside Eventually, once we've gotten SSA counting to a place where the oracle check consistently passes, I'd like to add some tests that utilize `oracle_check |
In #44557, we fixed some places where SSAValue uses were miscounted. During the development of that PR, after an egregious amount of
printlns, we devised a framework to test our calculated use counts against an "oracle" use count independently derived from the relevantIncrementalCompact.This PR cleans that framework up a bit and adds an option to toggle it (its off by default of course). Hopefully this framework means that no one has to manually check these counts again if/when we decide to make related changes.
Turning this on during our compiler tests shows we do still overcount at times. Overcounting is a lot less bad than undercounting (which was the crux of an issue with #44557 where we were deleting actually used instructions), but it may mean that we are missing some opportunities to do dead code elimination.
For example, the following comes almost immediately after starting
Base.runtests("compiler"):In this case, our oracle shows that
%13 = :(Core.tuple(%11, %12))is unused (which we can visually confirm), but it has not been eliminated by DCE because we think it has a use somewhere.