-
Notifications
You must be signed in to change notification settings - Fork 188
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
Compiler: lower level switch #1498
Conversation
let h = Hashtbl.create 16 in | ||
Array.iteri | ||
~f:(fun i (pc, _) -> | ||
Hashtbl.replace h pc (i :: (try Hashtbl.find h pc with Not_found -> []))) |
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.
@vouillon, how important is the optimization about known_cases ? do we need to restore it ?
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've tried to restore the optimization but it appears that our test-suite doesn't capture/test this.
Would you be able to contribute an expect test for this ?
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'm not sure it is really useful in practice. It makes the analysis more precise and does not cost much, but it's not clear to me whether it make much of a difference.
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.
Your change looks fine. I need to think of a test.
case 13: | ||
switch$0 = 3; break; | ||
switch$0 = 3; switch$1 = 1; break; |
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.
The diff in this file may look like a regression (in term of code size) but it looks much better once rebased on top of #1496
fix #1495