-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
feat(compiler): Allow disabling Binaryen optimizations #780
Conversation
@@ -33,6 +34,13 @@ let gensym_label = s => { | |||
}; | |||
let reset_labels = () => gensym_counter := 0; | |||
|
|||
let needs_exceptions = ref(false); |
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 the biggest fan of this approach, but it was definitely the simplest. The alternatives I thought of were another compilation mode specifically for exceptions, and registering imports in compcore.re
as they're used (so not just for exceptions, but for everything). I think the latter would be better than this, but it'd add quite a bit of complexity to implement it. If folks really don't like how I did it though, I can explore doing it that way.
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 don't think it should block this PR, but I think that we should open an issue to take the latter approach. Maybe also add a comment explaining why it's here? I feel like it could become a mystery variable very quickly (at least to me).
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.
Added a comment 👍
@@ -1,16 +1,29 @@ | |||
open Anftree; |
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 updated this optimization to drop unused imports, which resolves cyclic dependencies on the unsafe wasmXX modules.
@@ -306,8 +306,25 @@ let option_conv = ((prsr, prntr)) => ( | |||
| Some(x) => prntr(ppf, x), | |||
); | |||
|
|||
let optimizations_enabled = | |||
toggle_flag(~doc="Disable optimizations.", ~names=["O0"], true); | |||
type optimization_level = |
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 went pretty plain for this, but I could see us instead doing something like:
| Optimizations_disabled
| Critical_Grain_optimizations
| Grain_optimizations
| Grain_and_Binaryen_optimizations
but I felt pretty unsure about doing it that way.
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 this is good, especially since it's more meant for compiler developers than end-users.
("0", Level_zero), | ||
("1", Level_one), |
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.
These are useful for compiler authors, but aren't really useful for end users (since the programs may not work). We could maybe consider removing these, but I'm not sure how we as compiler authors would access these for development.
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.
We could maybe consider removing these, but I'm not sure how we as compiler authors would access these for development.
Sorry, I'm not sure what you mean by this? I'm not sure I understand the issue
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.
-O0
and -O1
are basically unsupported, so I was saying we could choose to only allow -O2
and -O3
. But to your point that it's largely for devs, it's probably fine.
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 like leaving us room to expand the usage of optimization levels (and thus keeping generic).
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.
Couple of things, but if you tell me they don't matter, here's an approval!
@@ -119,7 +119,7 @@ program | |||
"don't automatically import the Grain Pervasives module" | |||
) | |||
.graincOption("-o <filename>", "output filename") | |||
.graincOption("--O0", "disable optimizations") | |||
.graincOption("-O <level>", "set the optimization level") |
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.
Should this default to level 3?
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.
It might be good to do that since it'd make the docs a bit nicer (you'd see the choices, and you'd see that 3 was the default). I was largely offloading the logic to grainc
so we wouldn't have to keep them in sync. But it's probably better to do the better UX version.
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, better UX. This will eventually go away so we shouldn't worry about keeping it in sync for a little while.
try(!Ident.find_same(ident, used_symbols^)) { | ||
| Not_found => get_comp_purity(value) | ||
switch (Ident.find_same_opt(ident, used_symbols^)) { | ||
| Some () => false |
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'd honestly prefer if this tbl didn't use unit so the Reason code didn't look weird. It could even just be 0
or something
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.
Unit feels the most semantic since we're using the hash table here more as a hash "set", but yeah it totally makes the code look a little off 😛
WDYT about just doing Some(_)
here?
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 like that better.
("0", Level_zero), | ||
("1", Level_one), |
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 like leaving us room to expand the usage of optimization levels (and thus keeping generic).
("2", Level_two), | ||
("3", Level_three), | ||
]), | ||
Level_three, |
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.
How do you want to keep the JS CLI and this value in sync. Feels weird to have it be optional up there but then default to lvl 3 optimizations
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, that was the idea behind just having it be optional in the JS CLI—no need to worry about updating in two places.
This PR allows Binaryen optimizations to be disabled. Binaryen optimizations were previously required because the compiler would emit false circular module references for runtime modules (since it's assumed that all modules need the runtime modules), and Binaryen would remove them since they were unused, fixing the problem.
I'll leave some comments throughout about some implementation details.
@peblair I went to add a test and I realized I couldn't because there wasn't a way to configure the Binayren optimizations off, so I just went ahead and made the changes to the flags that we talked about in your other PR.