-
-
Notifications
You must be signed in to change notification settings - Fork 22.4k
[Web] Add memory64
option to setup the foundations of wasm64
#105670
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
base: master
Are you sure you want to change the base?
Conversation
WebXR almost certainly doesn't work with wasm64 because of the way we're passing data between JS and C++. But that's something that I'll need to work on in a future PR :-) |
I modified my PR to remove // in `platform/web/os_main.h`
bool is_memory64() const { return sizeof(size_t) == sizeof(uint64_t); } |
If we can know at compile time, why use a runtime check? |
Yeah. That's a compelling argument. But I could force inlining the function and the optimizer will surely optimize the crap out of these static checks. I'm just worried to introduce a Edit: I just did add |
@@ -73,6 +73,8 @@ class OS_Web : public OS_Unix { | |||
// Override return type to make writing static callbacks less tedious. | |||
static OS_Web *get_singleton(); | |||
|
|||
_FORCE_INLINE_ bool is_memory64() const { return sizeof(size_t) == sizeof(uint64_t); } |
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.
Is this true
or false
with wasm32+64bitptrs
?
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.
Is this
true
orfalse
withwasm32+64bitptrs
?
This is true
. The runtime is wasm32
, but the pointer sizes are 64-bit wide. The previous WASM_MEMORY64_ENABLED
define was applied to wasm32+64bitptrs
.
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.
Is this
true
orfalse
withwasm32+64bitptrs
?This is
true
. The runtime iswasm32
, but the pointer sizes are 64-bit wide. The previousWASM_MEMORY64_ENABLED
define was applied towasm32+64bitptrs
.
This is why I specified the option as +64bitptrs
instead of choosing a simpler, but way more cryptic wasm32+64
(which would imply wasm32+wasm64
, which is not the case).
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.
Mmm. Makes me reconsider the use of actual name of the function is_memory64()
, as it may be misleading. Technically, wasm32+64bitptrs
isn't truly MEMORY64.
My usual inclination is to not trust the compiler. :-) But this isn't my area of expertise, so I'm happy to trust more knowledgeable folks
Personally, I think, yes, these should use the suffix I think we'll also need feature tags for these different types, because I think GDExtensions will also need to be compiled the same way as Godot in order to work together, and we'll need a way for Godot to select the right binary. Example:
|
Technically, Edit: OH. GDExtensions, right. Forgot about this. I will test the compatibility between them. |
Scope changed and will require another review.
Even if wasm64 is not the silver bullet, browsers begin to support it, so we may as well begin work to officially support builds.
The new
memory64
option has 3 values (and some aliases).wasm32
(default, aliases:0
,no
)wasm64
(aliases:1
,yes
)wasm32+64bitptrs
(aliases:2
)0
,1
, and2
aliases correspond to-sMEMORY64
values for Emscripten. (see that link for thewasm32+64bitptrs
option explanation)Forwasm64
andwasm32+64bitptrs
, theWASM_MEMORY64_ENABLED
define will be set.Currently, actually using
MEMORY64
seems to break our current JS logic (asBigInt
arithmetic don't like to be mixed with standard numbers). Sowasm64
andwasm32+64bitptrs
builds wont actually technically work out of the box.I'm doing this PR right now because I want to setup the grounds for supporting the feature in future PRs,
especially with the help ofWASM_MEMORY64_ENABLED
.I currently have a feature currently baking that will need to know which type of memory we're dealing with.
Edit: I modified the PR to introduce instead
bool OS_Web::is_memory64()
.