-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[Wasm64] Fix EM_ASM for addresses >4GB addresses #19980
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
c5fcc8d
to
84c65a4
Compare
8387b69
to
04529b3
Compare
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 understand this uses makeGetValue to make sure to emit the right code, but what was breaking in the old code? We do support direct HEAPF64[..]
etc. access in wasm64, don't we? And >>=
should work on an int53?
I believe the problem with using |
Indeed any usage of bitshifting doesn't work for these larger addresses, right? |
Ah, yes, that does make sense. We'd need Another option might be |
Whats wrong with the approach taken here which uses the getVale/setValue helpers to avoid this completely? In most cases I think its more consistent to always use these helpers rather than direct heap access, no? |
1 similar comment
Whats wrong with the approach taken here which uses the getVale/setValue helpers to avoid this completely? In most cases I think its more consistent to always use these helpers rather than direct heap access, no? |
This approach is good. It's just the 11 byte code cost. Not a big deal but if this pattern happens in many places perhaps it adds up, that's what worries me. And I don't see obvious downsides to |
OK I switched to using division, which actually saves a few bytes. How confident are you the optimizing compiler can make these equivalent? |
I'm very confident VMs optimize this based on what I recall from years ago, but my information isn't recent, so we should probably verify to make sure. If we verify, this PR lgtm, but good point about that other PR with the macros, perhaps that is worth considering as well - what do you think? I lean towards just doing |
I reversed course again and decided to go with the helper macros here.. I think its cleaner and more consistent. And worth the 6 bytes of code size. It also unblocks a lot of followup changes to get >4gb tests passing. |
We can look into the possibility of using division rather than shifting if/when we have some performance numbers, but using the macros means that we can update in single location, making these measurements easier. |
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.
lgtm. if we find this extra code size appears in more places we can maybe prioritize focusing on it more then. if it's just a few places then the simplicity and other benefits seem worth it.
This change use makeGetValue helper macros to avoid direct heap access. This does come with a 6 byte code size cost (I measured the size of the JS code generated for wasm64.test_em_asm with -O2 going from 9275 to 9281).
This change use makeGetValue helper macros to avoid direct heap access.
This does come with a 6 byte code size cost (I measured the size of the
JS code generated for wasm64.test_em_asm with -O2 going from 9275 to
9281).