-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Explicitly set bulk memory and nontrapping-fptoint clang flags #22751
base: main
Are you sure you want to change the base?
Conversation
'MIN_SAFARI_VERSION', | ||
'MIN_CHROME_VERSION', | ||
'MIN_FIREFOX_VERSION', | ||
'MIN_NODE_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.
Hmm... this is kind of shame, but I guess its needed.
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 agreed; but if we don't want to just follow the default at compile time and have consistent feature control at both compile and link time I don't really see another 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.
Actually, once these features are on by default and we are just lowering them away in Binaryen when disabled, we could pull this out again, as long as we don't care about older LLVM versions that might have different defaults.
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.
Yes, good idea, lets always enable these features and compile time and then we only need to worry about lowering at link time.
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 actually think it might be clearer to get the lowering passes working first, otherwise we start to allow these settings at compile time and the try to dis-allow them later, once the lowering pass makes them irrelevant.
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.
That would be ideal; I think i have nontrapping-fp working and am partway through bulk-memory; we'd also need to be ready to ensure that all of our tests pass in that mode before they're ready, so it's good to have this in the back pocket if the change lands upstream before it's ready.
newargs += ['-mbulk-memory'] | ||
settings.BULK_MEMORY = 1 | ||
else: | ||
newargs += ['-mno-bulk-memory'] |
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 code in link.py should probably be moved here:
if not settings.BULK_MEMORY:
settings.BULK_MEMORY = feature_matrix.caniuse(feature_matrix.Feature.BULK_MEMORY)
Then once the setting is determines we should set the command line flag based on that.
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.
Does the code here in emcc.py run at link time too? In that case we could maybe just delete the code in link.py entirely?
Or would we still need it for the case where emcc.py isn't used at all?
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.
Yes, emcc.py runs at both compile and link time. If code needs to run at both compile and line time it should live there. If its only needs to run at link time it should live in link.py
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.
Can we somehow consolidate the different places where BULK_MEMORY
is set and -mno-bulk-memory
/ -mbulk-memory
are added.
At the very least I don't think we need the caniuse in link.py and in emcc.py both.
Perhaps this block along with the block above for handling shared memory should just set BULK_MEMORY
? Then we can use the BULK_MEMORY
setting along to set the command line flag?
Also /cc @walkingeyerobot since once it's finished this change will probably need to be mirrored for the bare-clang/non-emcc.py scenario. |
OK, assuming the tests are good, I think this should work to keep the feature set stable even as LLVM updates the defaults. Once we get the lowering passes in, and ensure our tests are correct, we can back this part out. |
I noticed these were a little out-of-place in emscripten-core#22751
I noticed these were a little out-of-place in #22751
When clang starts enabling features by default that Emscripten's minimum browser versions want disabled, Emscripten will need to explicitly disable them. This change allows the feature matrix to be used at compile time, and explicitly sets the use of bulk memory one way or the other.
See also llvm/llvm-project#112049