Skip to content
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

Use C/C++ bool type to implement EM_BOOL #22157

Merged
merged 2 commits into from
Jun 28, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jun 27, 2024

This reduces the size of several structs and can result in code size savings in some cases. The reason
the code size savings don't show up in trivial examples is (I believe) because this change also increases
the use of HEAP8 (where previously some examples only depended on HEAP32).

This change is split of from a larger change I have planned to remove the use of EM_BOOL completely: #22155.

@sbc100 sbc100 requested a review from kripken June 27, 2024 18:58
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 27, 2024

I think we should probably do a release containing this change before we land part 2 so that we can separate any possible fallout.

@sbc100 sbc100 requested a review from dschuff June 27, 2024 18:58
@sbc100 sbc100 force-pushed the em_bool_part1 branch 3 times, most recently from b4b6a8c to 019491a Compare June 27, 2024 21:13
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 27, 2024

I had to update a bunch of places where we were accessing these fields (u8 vs u32 access). PTAL.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 27, 2024

I used the following commands to fine and fix all locations in the JS library code that was treating these values as 32 bit:

# Find all EM_BOOL struct members
$ git grep "^\s*EM_BOOL[^(]*$" system/include/ | awk '{ print $3 }' | sed "s/;//" | sort | uniq > bools.txt
# Find all places where these members are used in HEAP32/HEAPU32 accesses
$ for bool in `cat bools.txt`; do git grep "HEAPU\?32.*$bool"; done
# Find all the places they are used in `i32`/`u32` `makeGetValue`/`makeSetValue`
$ for bool in `cat bools.txt`; do git grep "make[GS]etValue.*\.$bool\>.*32"; done

This reduces the size of several structs and results in code size
savings in some cases.

This change is split of from a larger change I have planned to remove
the use of EM_BOOL completely: emscripten-core#22155.
I used the following commands to make sure I got all of them this time.

$ git grep "^\s*EM_BOOL[^(]*$" system/include/ | awk '{ print $3 }' | sed "s/;//" | sort | uniq > bools.txt
$ for bool in `cat bools.txt`; do git grep "make[GS]etValue.*\.$bool\>.*32"; done
@sbc100 sbc100 requested a review from kripken June 28, 2024 17:06
@sbc100 sbc100 merged commit 2dc74c8 into emscripten-core:main Jun 28, 2024
28 checks passed
@sbc100 sbc100 deleted the em_bool_part1 branch June 28, 2024 17:06
sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2024
sbc100 added a commit to sbc100/emscripten that referenced this pull request Jul 8, 2024
sbc100 added a commit that referenced this pull request Jul 8, 2024
verhovsky pushed a commit to verhovsky/emscripten that referenced this pull request Jul 30, 2024
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 20, 2024
…#22157 that changed size of EM_BOOL from 4 bytes down to one byte. Closes #46.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Sep 3, 2024
These macros were already updated to expand to the C/C++ `bool` type
back in emscripten-core#22157.  This is the natural NFC followup to that change.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 1, 2024
These macros were already updated to expand to the C/C++ `bool` type
back in emscripten-core#22157.  This is the natural NFC followup to that change.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 1, 2024
These macros were already updated to expand to the C/C++ `bool` type
back in emscripten-core#22157.  This is the natural NFC followup to that change.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Oct 1, 2024
These macros were already updated to expand to the C/C++ `bool` type
back in emscripten-core#22157.  This is the natural NFC followup to that change.
sbc100 added a commit that referenced this pull request Oct 2, 2024
These macros were already updated to expand to the C/C++ `bool` type
back in #22157.  This is the natural NFC followup to that change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants