Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} #19289

juj · 2023-05-03T20:30:02Z

Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} and {{{ isPtrAligned() }}} to help 2GB, 4GB and MEMORY64 memory modes. Remove {{{ to64() }}} and {{{ from64() }}} since they had an awkward mismatch of in-place vs new returned expression styles.

This is a looser form of PR #15433, and it embraces the idea that pointer values in JavaScript side will not need to retain the full 64-bit values, but can be truncated to 53 bits. In other words, pointers can always be leisurely converted to Numbers in JS side.

The idea of this new function family will be to be public to JS library authors, so looking to add API tests and documentation to accommodate this PR, if people agree on the contents?

The main motivation is to provide a single mechanism to deal with 64-bit pointers (and size_t) in a way that works in efficient code-size manner (no redundant x = x; expressions, shifts or casts in any build mode) and gives developers a single API to allow covering each of 2GB, 4GB and MEMORY64 build modes.

juj · 2023-05-08T15:01:54Z

Ping @kripken @sbc100 would you have a moment to review this?

kripken · 2023-05-11T23:45:03Z

index (an unsigned JS Number)

Perhaps "index" could be "i53"? That seems more clear than either index or number, I think, as i53 makes it obvious it is an integer, but it's in 53 bits so it's stored in a JS number.

convertPtrToIndex

How about "convert" => "update", since it updates the value in place?

sbc100

Sorry, these comments are maybe old but I thought I would send them anyway.. I still ned to revisit this since you updated some stuff.

sbc100 · 2023-05-03T21:14:28Z

src/library.js

@@ -194,7 +194,7 @@ mergeInto(LibraryManager.library, {
  emscripten_resize_heap: 'ip',
  emscripten_resize_heap: function(requestedSize) {
    var oldSize = HEAPU8.length;
-    requestedSize = requestedSize >>> 0;
+    {{{ convertPtrToIdx('requestedSize') }}};


How about just dropping the convert and just calling this ptrToIdx? I'm also not very clear that "Index" is the right thing to be calling this. I don't love either of these but what about ptrToNumber or wasmToJSPtr ?

Because emscripten_resize_heap has its argument marked as p I don't think we need the full conversion here do we? i.e. we didn't need from64 here before.

Having a separate convertPtrToIdx('variable') function has the benefit that it avoids a redundant assignment to self in wasm2gb builds. I.e. if one writes

variable = {{{ ptrToIdx('variable') }}};

Then it will generate variable = variable; in <2GB builds, and one will need to use Closure to fix that up.

sbc100 · 2023-05-03T21:18:46Z

src/library.js

@@ -3128,7 +3128,7 @@ mergeInto(LibraryManager.library, {
  // Used by wasm-emscripten-finalize to implement STACK_OVERFLOW_CHECK
  __handle_stack_overflow__deps: ['emscripten_stack_get_base', 'emscripten_stack_get_end', '$ptrToString'],
  __handle_stack_overflow: function(requested) {
-    requested = requested >>> 0;
+    {{{ convertPtrToIdx('requested') }}};


Again, I think the incoming param is already a number so conversion from BigInt here is not needed?

Doesn't there need to be a __handle_stack_overflow__sig for the automatic conversion to occur? There isn't one for __handle_stack_overflow?

sbc100 · 2023-05-03T21:19:33Z

src/library.js

    // Function pointers are 64-bit, but wasmTable.get() requires a Number.
    // https://github.com/emscripten-core/emscripten/issues/18200
-    funcPtr = Number(funcPtr);
-#endif
+    {{{ convertPtrToIdx(funcPtr);


I think this line can be removed because funcPtr should already have been converted to a number when was first passed to JS.

src/parseTools.js

src/runtime_safe_heap.js

sbc100

Sorry, these comments are maybe old but I thought I would send them anyway.. I still ned to revisit this since you updated some stuff.

sbc100

Sorry, these comments are maybe old but I thought I would send them anyway.. I still ned to revisit this since you updated some stuff.

sbc100 · 2023-06-14T18:30:42Z

src/library_html5_webgl.js

@@ -78,10 +78,11 @@ var LibraryHtml5WebGL = {
  '$JSEvents', '$emscripten_webgl_power_preferences', '$findEventTarget', '$findCanvasEventTarget'],
  // This function performs proxying manually, depending on the style of context that is to be created.
  emscripten_webgl_do_create_context: function(target, attributes) {
+    {{{ convertPtrToIdx('target') }}};


Again, because emscripten_webgl_do_create_context has it signature set in library_sigs.js to ipp these incoming pointers should already have been converted (at least in the wasm64 case). This should be handled automatically I believe.

Hmm, it wasn't.. I was getting a crash here in Unity, and had to add this here to fix.

Perhaps that was an older version before the __sig attribute was added? If you look at the generated code for emscripten_webgl_do_create_context you should see that it has auto-generated conversion code that is generated by convertPointerParams in src/jsifier.js.

sbc100 · 2023-08-08T21:15:04Z

src/library.js

    // magic here.  This is because the __sig wrapper uses arrow function
    // notation which causes the inner call to traverseStack to fail.
-    {{{ from64('str') }}};
+    {{{ convertPtrToIdx('str') }}};


This should no longer be needed since #19820 landed.

sbc100 · 2023-08-08T21:24:31Z

After working on #19980 I think something like {{{ ptrToIdx }}} likely needed after all.

I'm not sure about the removal of to64 and from64 though.. or at least don't see them as linked to need for ``{{{ ptrToIdx }}}. I'd like to keep the concepts of coveting to/from BitInt separate from the concept of indexing I think. They seem fairly logically seperate to me. The bigint/number stuff only happens at the boundary with native code.. where as the {{{ ptrToIdx }}}` is used to convert numbers to different index spaces.

If you have time would you like to update this PR, perhaps with the to64/from64 stuff kept separate? Otherwise, I'd be up for carving more parts of this out and landing in chunks if that is OK with you?

juj · 2023-08-09T16:00:26Z

I landed this ptrToIdx PR to our Unity Emscripten branch in June, and verified its behavior against Unity in 4GB mode. That uncovered a number of failures in tests, which I have been now in the process of going through.

I still would like to land these types of macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} into general Emscripten JS library use, I think they are the right type of abstraction that allows users to manually control the exact codegen. These could be paired with the automatic __sig-based type conversion for use cases where people are ok to rely on automatic (but maybe slightly code size pessimized) conversions and do not feel as pedantic about code size as we do.

sbc100 · 2023-08-09T16:13:02Z

I landed this ptrToIdx PR to our Unity Emscripten branch in June, and verified its behavior against Unity in 4GB mode. That uncovered a number of failures in tests, which I have been now in the process of going through.

I still would like to land these types of macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} into general Emscripten JS library use, I think they are the right type of abstraction that allows users to manually control the exact codegen. These could be paired with the automatic __sig-based type conversion for use cases where people are ok to rely on automatic (but maybe slightly code size pessimized) conversions and do not feel as pedantic about code size as we do.

I agree that ptrToIdx is generally useful to converting to heap indexes, but I'd woul like to be able to separate the BigInt/Number conversions (which generally just need to happen on the boundary), with the shifting operations (that need to happen repeatedly and at arbitrary points.

When reading the code I think its useful to be able to distinguish between these to cases.

Receiving a pointer from native code and converting it to our internal convention of i53
Shifting a pointer for use in as a typed array index.

I can see that it might sometimes be useful to combine these two when it makes sense.

Note that (1) is no-op on wasm32

Most of our JS library can save on code size by assuming they are always working with numbers (i53) and that i53 conversions has already been performed. i.e. it only needs to worry about (2). So I think it makes sense to make (1) and (2) distinct operations in the macro system.

juj · 2023-08-09T16:27:35Z

Hmm, can you give an example of the kind of code where it would be beneficial to separate the two? I am not sure I follow. I mean, certainly could be done but my mind is drawing blank on what the code construct/patterns with that would be intended to look like, or how that would be used best?

sbc100 · 2023-08-09T22:01:29Z

Hmm, can you give an example of the kind of code where it would be beneficial to separate the two? I am not sure I follow. I mean, certainly could be done but my mind is drawing blank on what the code construct/patterns with that would be intended to look like, or how that would be used best?

Any function that takes a pointer and then does a bunch of shift heap accesses using that pointer would only want to do the i53 convertion (cast of Number) once .. and not for each heap access. If we to do the conversions once then heap accesses (which are more common) should be smaller.

Another way of putting it: if possible, we would not want to do the i53 conversion (cast to Number) more than once for any given incoming or outgoing pointer.

…dxToPtr() }}} to help 2GB, 4GB and MEMORY64 memory modes. Remove {{{ to64() }}} and {{{ from64() }}} since they had an awkward mismatch of in-place vs expression styles. This is a looser form of PR emscripten-core#15433, and it embraces the idea that pointer values in JavaScript side will not need to retain the full 64-bit values, but can be truncated to 53 bits. Update library_html5.js for >2GB Address review and fix convertPtrToIdx on non-byte shifts

juj · 2023-08-29T10:54:38Z

Any function that takes a pointer and then does a bunch of shift heap accesses using that pointer would only want to do the i53 convertion (cast of Number) once .. and not for each heap access. If we to do the conversions once then heap accesses (which are more common) should be smaller.

Shouldn't such code want to do the shift heap accesses also only once? Multiple shifts would be redundant?

juj · 2023-08-29T10:55:23Z

if possible, we would not want to do the i53 conversion (cast to Number) more than once for any given incoming or outgoing pointer.

I agree - also I think we should not do the shifts more than once for a pointer. So in my view having the common ptrToIdx() resolve both the BigInt->Number and shift conversion would make sense?

juj · 2023-08-29T14:05:23Z

Found a bug in Chrome with WebGL 2 around > 2GB WebGL support: https://bugs.chromium.org/p/chromium/issues/detail?id=1476859 . CC @kenrussell

sbc100 · 2023-08-29T16:37:43Z

I agree - also I think we should not do the shifts more than once for a pointer. So in my view having the common ptrToIdx() resolve both the BigInt->Number and shift conversion would make sense?

The things is that in almost all cases the convertion to Number happens in the automatic wrapper generated by __sig.

The convertion to heap offset (e.g. HEAP8 or the HEAPU32) I agree would ideally only happen once, but a given function might want to access other the HEAP8 and HEAPU32 via the same pointer, so several shifts might still be needed in a single function, even if we try make just one per type.

BTW, I've been making some good progress on this using the existing getHeapOffset function (which I think its nice because the second argument is a type (e.g. i32) rather than a number that the reader has to know is bitshift:

emscripten/src/parseTools.js

Lines 294 to 304 in 7858816

    
           function getHeapOffset(offset, type) { 
        
             const sz = getNativeTypeSize(type); 
        
             const shifts = Math.log(sz) / Math.LN2; 
        
             if (MEMORY64 == 1) { 
        
               return `((${offset})/${2 ** shifts})`; 
        
             } else if (CAN_ADDRESS_2GB) { 
        
               return `((${offset})>>>${shifts})`; 
        
             } else { 
        
               return `((${offset})>>${shifts})`; 
        
             } 
        
           }

Isn't ptrToIdx mostly duplicating the functionality of this existing helper?

One problem with the "try to only do one shift per function" is that a lot of our existing code relies on makeGetValue and makeSetValue for heap access and those function will generate a shift for every access. We could make variants of those that use a pre-shifted values, as an optimization... but that seems separate to the issue at hand here.

sbc100 · 2023-08-29T16:39:58Z

BTW did you see that we can now run the entire core test suite in CAN_ADDRESS_2GB mode and also in a mode where all addresses are over 4GB!

Aside from a couple of exceptions all the test now pass after a bunch of changes in the last few weeks. See:

emscripten/test/test_core.py

Lines 9893 to 9894 in 7858816

    
           wasm64_4gb = make_run('wasm64_4gb', emcc_args=['-Wno-experimental', '--profiling-funcs'], 
        
                                 settings={'MEMORY64': 1, 'INITIAL_MEMORY': '4200mb', 'GLOBAL_BASE': '4gb'},

emscripten/test/test_core.py

Lines 9882 to 9884 in 7858816

    
           # Test >2gb memory addresses 
        
           core_2gb = make_run('core_2gb', emcc_args=['--profiling-funcs'], 
        
                               settings={'INITIAL_MEMORY': '2200mb', 'GLOBAL_BASE': '2gb'})

juj · 2023-08-29T17:10:18Z

Ah I see, you have landed a new getHeapOffset() function.

The things is that in almost all cases the convertion to Number happens in the automatic wrapper generated by __sig.

Relying on a combination of __sig + getHeapOffset() has some shortcomings that I can think of.

First is that it requires using the __sig mechanism for all functions that use pointers to make them work. I don't think that is desirable. Sure, we can have a __sig method for some uses, but the code that is generated for wasm64 mode with that combo seems a bit awkward. For a JS library function

addToLibrary({
	foo__sig: 'vp',
	foo: (ptr) => {
		HEAPU32[{{{ getHeapOffset('ptr', 'u32') }}}] = 42;
	}
});

it generates

var bigintToI53Checked = num => (num < MIN_INT53 || num > MAX_INT53) ? NaN : Number(num);
function _foo(ptr) {
 ptr = bigintToI53Checked(ptr);
 HEAPU32[((ptr) / 4)] = 42;
}

With ptrToIdx, i.e.

addToLibrary({
	foo: (ptr) => {
		HEAPU32[{{{ ptrToIdx('ptr', 2) }}}] = 42;
	}
});

one will get a better output that reads

var _foo = ptr => {
 HEAPU32[Number(ptr >> 2n)] = 42;
};

which to me is the optimal form for such an access. No extra function calls or divisions. I would advise against having that division in there. (maybe for -Oz builds we could transform a Number(ptr >> 2n) into a Number(ptr)/4 and say that it is not "Wasm53" but rather "Wasm50").

Note btw that the effect of bigintToI53Checked is a no-op at the moment. Or rather, it will cause a new property key undefined to be added on the HEAPU32 object if the memory pointer is invalid. (edit: actually sorry, it doesn't. Only doing HEAPU32[undefined]=42; would add that; looks like HEAPU32[NaN]=42; does get just ignored)

Another suboptimality with __sig is that if you have a function that only holds a pointer and does not do anything with it, except to pass it back to another function back to wasm (prime example being a userData pointer), it is counterproductive to first cast it from BigInt to Number at Wasm->JS ingress, and then from a Number back to BigInt again at JS->Wasm egress. The value can just be held passed through unmodified. But the __sig mechanism does not allow doing that, and it is restricted to either wholesale converting all pointer parameters to a function, or none of them. (or then we'd want to have a second character to denote a pointer-but-do-not-convert).

juj · 2023-08-29T17:18:43Z

BTW did you see that we can now run the entire core test suite in CAN_ADDRESS_2GB mode and also in a mode where all addresses are over 4GB!

This is superb. I am running against this now, and checking also some WebGL builds with -sGLOBAL_BASE=2GB.

Although in general, I wonder if there may be wins to be had by assuming that global static data section pointers will always be < 2GB pointers.

sbc100 · 2023-08-29T18:07:10Z

Ah I see, you have landed a new getHeapOffset() function.

The things is that in almost all cases the convertion to Number happens in the automatic wrapper generated by __sig.

Relying on a combination of __sig + getHeapOffset() has some shortcomings that I can think of.

First is that it requires using the __sig mechanism for all functions that use pointers to make them work. I don't think that is desirable. Sure, we can have a __sig method for some uses, but the code that is generated for wasm64 mode with that combo seems a bit awkward. For a JS library function
addToLibrary({
	foo__sig: 'vp',
	foo: (ptr) => {
		HEAPU32[{{{ getHeapOffset('ptr', 'u32') }}}] = 42;
	}
});
it generates
var bigintToI53Checked = num => (num < MIN_INT53 || num > MAX_INT53) ? NaN : Number(num);
function _foo(ptr) {
 ptr = bigintToI53Checked(ptr);
 HEAPU32[((ptr) / 4)] = 42;
}
With ptrToIdx, i.e.
addToLibrary({
	foo: (ptr) => {
		HEAPU32[{{{ ptrToIdx('ptr', 2) }}}] = 42;
	}
});
one will get a better output that reads
var _foo = ptr => {
 HEAPU32[Number(ptr >> 2n)] = 42;
};
which to me is the optimal form for such an access. No extra function calls or divisions. I would advise against having that division in there. (maybe for -Oz builds we could transform a Number(ptr >> 2n) into a Number(ptr)/4 and say that it is not "Wasm53" but rather "Wasm50").

Note btw that the effect of bigintToI53Checked is a no-op at the moment. Or rather, it will cause a new property key undefined to be added on the HEAPU32 object if the memory pointer is invalid. (edit: actually sorry, it doesn't. Only doing HEAPU32[undefined]=42; would add that; looks like HEAPU32[NaN]=42; does get just ignored)

Another suboptimality with __sig is that if you have a function that only holds a pointer and does not do anything with it, except to pass it back to another function back to wasm (prime example being a userData pointer), it is counterproductive to first cast it from BigInt to Number at Wasm->JS ingress, and then from a Number back to BigInt again at JS->Wasm egress. The value can just be held passed through unmodified. But the __sig mechanism does not allow doing that, and it is restricted to either wholesale converting all pointer parameters to a function, or none of them. (or then we'd want to have a second character to denote a pointer-but-do-not-convert).

It sounds like you are arguing for allowing pointers to flow in the JS as bigint value whereas the current approach that I've been aiming for is to assume pointers a numbers everywhere in JS (except on the boundary).

I don't mind revisiting that debate, but perhaps after we have all tests passing in the current mode? I hope that the abstractions we are using today makeGetValue, makeSetValue , getHeapOffset and __sig attributes should allow us to experiment with both options without massive code changes. What I'd like to avoid, if possible, is mix of both where some JS code accepts "pointer-as-number" and other JS code accepts "pointer-as-bigint".

sbc100 · 2023-08-29T18:10:27Z

Ah I see, you have landed a new getHeapOffset() function.

I updated getHeapOffset() to handle MEMORY64 and CAN_ADDRESS_2GB, but its been around forever as far as I can tell (since 2011: 5b2f181).

sbc100 · 2024-01-31T01:06:35Z

I'm now working to fix the long tail of wasm64 issues. See #21177.

I'm running into issues that this PR addresses. I'm not sure if we should try to land this PR but I'm certainly using it as inspiration for my changes.

juj force-pushed the ptr_to_idx_and_back branch 2 times, most recently from 41551a4 to 4710fe3 Compare May 4, 2023 11:48

juj force-pushed the ptr_to_idx_and_back branch from 47f4481 to 8267438 Compare May 11, 2023 19:44

sbc100 reviewed May 12, 2023

View reviewed changes

sbc100 added the wasm64 label Jun 7, 2023

sbc100 reviewed Jun 14, 2023

View reviewed changes

sbc100 mentioned this pull request Jul 12, 2023

How does emscripten generate 32-bit and 64-bit wasm #19833

Closed

sbc100 mentioned this pull request Aug 8, 2023

[Wasm64] Fix EM_ASM for addresses >4GB addresses #19980

Merged

sbc100 reviewed Aug 8, 2023

View reviewed changes

juj force-pushed the ptr_to_idx_and_back branch from 8809d51 to b5f597f Compare August 29, 2023 10:52

More WebGL updates

d417758

juj added 2 commits August 29, 2023 17:05

Fix merge issues

3593b67

Work around Firefox and Chrome bugs

64d50cd

Remove uses of CAN_ADDRESS_2GB

7d48ac1

Rebaseline

e97af5a

juj force-pushed the ptr_to_idx_and_back branch from ca62199 to e97af5a Compare August 29, 2023 17:43

juj added 2 commits August 29, 2023 20:49

Fix conflict with closure and export wrapper generation in webgl

cc8d42f

Fix bad check

6a2efe2

Rebaseline

1805e3a

juj force-pushed the ptr_to_idx_and_back branch from 05da82a to 1805e3a Compare August 29, 2023 18:12

juj added 2 commits August 30, 2023 10:27

Use idxToPtr

ce19bdb

Merge remote-tracking branch 'origin/main' into ptr_to_idx_and_back

ac9fce5

juj mentioned this pull request Oct 15, 2024

Fix audio worklet + memory64 #22731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} #19289

Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} #19289

juj commented May 3, 2023 •

edited

Loading

juj commented May 8, 2023

kripken commented May 11, 2023 •

edited

Loading

sbc100 left a comment

sbc100 May 3, 2023

sbc100 May 3, 2023

juj Aug 29, 2023

sbc100 May 3, 2023

juj Aug 29, 2023

sbc100 May 3, 2023

sbc100 left a comment

sbc100 left a comment

sbc100 Jun 14, 2023

juj Jun 14, 2023

sbc100 Jun 14, 2023

sbc100 Aug 8, 2023

sbc100 commented Aug 8, 2023

juj commented Aug 9, 2023 •

edited

Loading

sbc100 commented Aug 9, 2023

juj commented Aug 9, 2023

sbc100 commented Aug 9, 2023

juj commented Aug 29, 2023

juj commented Aug 29, 2023

juj commented Aug 29, 2023

sbc100 commented Aug 29, 2023 •

edited

Loading

sbc100 commented Aug 29, 2023

juj commented Aug 29, 2023 •

edited

Loading

juj commented Aug 29, 2023 •

edited

Loading

sbc100 commented Aug 29, 2023

sbc100 commented Aug 29, 2023

sbc100 commented Jan 31, 2024

Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} #19289

Are you sure you want to change the base?

Implement macros {{{ ptrToIdx() }}}, {{{ convertPtrToIdx() }}}, {{{ idxToPtr() }}} #19289

Conversation

juj commented May 3, 2023 • edited Loading

juj commented May 8, 2023

kripken commented May 11, 2023 • edited Loading

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Aug 8, 2023

juj commented Aug 9, 2023 • edited Loading

sbc100 commented Aug 9, 2023

juj commented Aug 9, 2023

sbc100 commented Aug 9, 2023

juj commented Aug 29, 2023

juj commented Aug 29, 2023

juj commented Aug 29, 2023

sbc100 commented Aug 29, 2023 • edited Loading

sbc100 commented Aug 29, 2023

juj commented Aug 29, 2023 • edited Loading

juj commented Aug 29, 2023 • edited Loading

sbc100 commented Aug 29, 2023

sbc100 commented Aug 29, 2023

sbc100 commented Jan 31, 2024

juj commented May 3, 2023 •

edited

Loading

kripken commented May 11, 2023 •

edited

Loading

juj commented Aug 9, 2023 •

edited

Loading

sbc100 commented Aug 29, 2023 •

edited

Loading

juj commented Aug 29, 2023 •

edited

Loading

juj commented Aug 29, 2023 •

edited

Loading