Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049
[WebAssembly] Enable nontrapping-fptoint and bulk-memory by default. #112049
Changes from 7 commits
c4327c9
8a655be
229888f
d757a4d
6f4d40e
d3e79ec
04b0211
543ee22
a119df5
548564b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we need to disable bulk memory here and in many other files?
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.
In lld/test/wasm/custom-section-name.ll,
it's because bulk-memory makes the target_features section bigger and thus changes some of the section offsets and it was easier to just disable bulk-memory than to figure out the new section offsets. I can update the section offsets if you prefer though.See my comment below.In lld/test/wasm/data-segments.ll, it's because the test is written to test both bulk-memory and non-bulk-memory, and to keep it doing that, it's now necessary to explicitly disable bulk-memory for the non-bulk-memory case.
In lld/test/wasm/lto/stub-library-libcall.s it's because the test is testing the use of a memcpy stub function, and with bulk-memory it gets inlined.
llvm/test/MC/WebAssembly/extern-functype-intrinsic.ll is similarly testing memset calls.
llvm/test/MC/WebAssembly/libcall.ll is similarly testing memcpy calls.
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.
Thanks for the explanation!
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 now investigated custom-section-name.ll more.
With bulk-memory, we get a
__wasm_init_memory
function:Without bulk-memory we get a .bss section:
The test is intended to test the use of the .bss section, and we only get a .bss section in this test if we disable 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.
Hm, interesting, so we are using passive segments anytime bulk memory is enabled. That makes sense since this was all meant for threads, but is this actually an improvement in the non-threads case? i.e. maybe we don't want the extra explicit initialization code if we can help it?
edit: i just checked this out but I'll leave it here FTR: it looks like after linking we actually don't use passive segments unless threads is also enabled. so this seems like not a problem.