Skip to content

Make makeSetValue rounding consistent with HEAP64 semantics #19239

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Make makeSetValue rounding consistent with HEAP64 semantics #19239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions src/parseTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ function splitI64(value) {
//
// $1$0 = ~~$d >>> 0;
// $1$1 = Math.abs($d) >= 1 ? (
// $d > 0 ? Math.min(Math.floor(($d)/ 4294967296.0), 4294967295.0)
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0,
Copy link
Member

@kripken kripken Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0,
// $d > 0 ? Math.floor(($d)/ 4294967296.0) >>> 0

// : Math.ceil(Math.min(-4294967296.0, $d - $1$0)/ 4294967296.0)
// ) : 0;
//
Expand All @@ -227,9 +227,8 @@ function splitI64(value) {
const high = makeInlineCalculation(
asmCoercion('Math.abs(VALUE)', 'double') + ' >= ' + asmEnsureFloat('1', 'double') + ' ? ' +
'(VALUE > ' + asmEnsureFloat('0', 'double') + ' ? ' +
asmCoercion('Math.min(' + asmCoercion('Math.floor((VALUE)/' +
asmEnsureFloat(4294967296, 'double') + ')', 'double') + ', ' +
asmEnsureFloat(4294967295, 'double') + ')', 'i32') + '>>>0' +
asmCoercion('Math.floor((VALUE)/' +
asmEnsureFloat(4294967296, 'double') + ')', 'double') + '>>>0' +
Copy link
Member

@kripken kripken Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it correct to remove the Math.min here?

I think the old code was correct. We can do rounding for the lower bits because we've ensured they are in the right range. But we need to use saturation because the value might be outside of range - imagine that the value is 2^70 plus some small value, then doing >>>0 will do an odd wrapping operation (which can make it "randomly" lower or higher).

Copy link
Collaborator Author

@sbc100 sbc100 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code did saturation of the upper half produced this odd result:

0x1122334455667788AA -> 0xffffffff66780000

The new code truncates and produces the same result as BigInt64Array:

0x1122334455667788AA -> 0x2233445566780000

Copy link
Collaborator Author

@sbc100 sbc100 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"imagine that the value is 2^70 plus some small value" .. I'm not sure I understand doesn't the Math.floor(($d)/ 4294967296.0) take case of removing all the low bits? Basically the same as >> 32 in C/C++?

Copy link
Member

@kripken kripken Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It removes the low bits, by my concern is that without saturation the loss abs(x - int64(x)) can be high. Saturation keeps the value as close as possible to the original, minimizing the absolute error/difference.

But maybe I'm wrong and that is not the error to care about?

What do you mean by BigInt64Array's behavior here? How are you checking that?

Copy link
Collaborator Author

@sbc100 sbc100 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run this test both with and without WASM_BIGINT:

  @also_with_wasm_bigint                                                                             
  def test_parseTools(self):                                                                            
    self.do_other_test('test_parseTools.c', emcc_args=['--js-library', test_file('other/test_parseTools.js')])

The change in the expected output was initially generated because I changed WASM_BIGINT to always use BigInt64Array... then I updated splitI64 such that its output produces the same result.

Copy link
Member

@kripken kripken Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what code path that is on? I'm surprised splitI64 is even used in WASM_BIGINT mode as nothing is legalized. Probably I'm forgetting something...

Copy link
Collaborator Author

@sbc100 sbc100 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

$ node
Welcome to Node.js v19.0.0.
Type ".help" for more information.
> const uint8 = new Uint8Array(2);
undefined
> uint8[0] = 300
300
> uint8[0]
44
>

Copy link
Member

@kripken kripken Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Maybe I'm confused as to where we use splitI64 then. I agree that when we store a bigger value into a small number of bits then we must naturally truncate. But splitI64, I thought, is used to approximate a number, not prepare it for storing.

If it is used to prepare for storing, then as I asked before, why is it even used in WASM_BIGINT mode? In that mode we can store directly.

Or is my confusion that it isn't used in that mode, and you fixed it for non-WASM_BIGINT mode?

Copy link
Collaborator Author

@sbc100 sbc100 Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Maybe I'm confused as to where we use splitI64 then. I agree that when we store a bigger value into a small number of bits then we must naturally truncate. But splitI64, I thought, is used to approximate a number, not prepare it for storing.

Perhaps splitI64 had other callsites in the past but today it has exactly one usage in makeSetValue and it used to split the incoming number for storage as a pair of I32.

If it is used to prepare for storing, then as I asked before, why is it even used in WASM_BIGINT mode? In that mode we can store directly.

After this change is not longer used in WASM_BIGINT mode and we using BigIntArray directly.

Prior to this change we used splitI64 only because there was some confusion about whether the target address would be 8 byte aligned.

Or is my confusion that it isn't used in that mode, and you fixed it for non-WASM_BIGINT mode?

Right the changes to splitI64 are so that the non-WASM_BIGINT mode writes the same bits that WASM_BIGINT mode would write.

Copy link
Member

@kripken kripken Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks, that all makes sense then!

' : ' +
asmFloatToInt(asmCoercion('Math.ceil((VALUE - +((' + asmFloatToInt('VALUE') + ')>>>0))/' +
asmEnsureFloat(4294967296, 'double') + ')', 'double')) + '>>>0' +
Expand Down Expand Up @@ -364,12 +363,10 @@ function makeSetValue(ptr, pos, value, type, noNeedFirst, ignore, align, sep) {
assert(typeof align === 'undefined', 'makeSetValue no longer supports align parameter');
assert(typeof noNeedFirst === 'undefined', 'makeSetValue no longer supports noNeedFirst parameter');
assert(typeof sep === 'undefined', 'makeSetValue no longer supports sep parameter');
if (type == 'i64' && (!WASM_BIGINT || !MEMORY64)) {
// If we lack either BigInt support or Memory64 then we must fall back to an
// unaligned read of a 64-bit value: without BigInt we do not have HEAP64,
// and without Memory64 i64 fields are not guaranteed to be aligned to 64
// bits, so HEAP64[ptr>>3] might be broken.
return '(tempI64 = [' + splitI64(value) + '],' +
if (type == 'i64' && !WASM_BIGINT) {
// If we lack either BigInt we must fall back to an reading a pair of I32
Copy link
Member

@kripken kripken Apr 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If we lack either BigInt we must fall back to an reading a pair of I32
// If we lack BigInt we must fall back to an reading a pair of I32

// values.
return '(tempI64 = [' + splitI64(value) + '], ' +
makeSetValue(ptr, pos, 'tempI64[0]', 'i32') + ',' +
makeSetValue(ptr, getFastValue(pos, '+', getNativeTypeSize('i32')), 'tempI64[1]', 'i32') + ')';
}
Expand Down
2 changes: 2 additions & 0 deletions src/preamble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,9 +1156,11 @@ function createWasm() {
#endif
}

#if !WASM_BIGINT
// Globals used by JS i64 conversions (see makeSetValue)
var tempDouble;
var tempI64;
#endif

#include "runtime_debug.js"

Expand Down
2 changes: 1 addition & 1 deletion src/preamble_minimal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function abort(what) {
throw {{{ ASSERTIONS ? 'new Error(what)' : 'what' }}};
}

#if SAFE_HEAP
#if SAFE_HEAP && !WASM_BIGINT
// Globals used by JS i64 conversions (see makeSetValue)
var tempDouble;
var tempI64;
Expand Down
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors1.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25939
25914
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_ctors2.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25903
25878
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
30453
30428
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_except_wasm.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25748
25723
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_mangle.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
30457
30432
2 changes: 1 addition & 1 deletion test/other/metadce/test_metadce_cxx_noexcept.jssize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
25939
25914
13 changes: 0 additions & 13 deletions test/other/test_parseTools.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ void test_makeGetValue(int64_t* ptr);
void test_makeSetValue(int64_t* ptr);
int test_receiveI64ParamAsI53(int64_t arg1, int64_t arg2);
int test_receiveI64ParamAsDouble(int64_t arg1, int64_t arg2);
void test_makeSetValue_unaligned(int64_t* ptr);

#define MAX_SAFE_INTEGER (1ll << 53)
#define MIN_SAFE_INTEGER (-MAX_SAFE_INTEGER)
Expand Down Expand Up @@ -55,18 +54,6 @@ int main() {
rtn = test_receiveI64ParamAsDouble(MIN_SAFE_INTEGER - 1, 0);
printf("rtn = %d\n", rtn);

printf("\ntest_makeSetValue_unaligned\n");
// Test an unaligned read of an i64 in JS. To do that, get an unaligned
// pointer. i64s are only 32-bit aligned, but we can't rely on the address to
// happen to be unaligned here, so actually force an unaligned address (one
// of the iterations will be unaligned).
char buffer[16];
for (size_t i = 0; i < 8; i += 4) {
int64_t* unaligned_i64 = (int64_t*)(buffer + i);
test_makeSetValue_unaligned(unaligned_i64);
printf("i64 = 0x%llx\n", *unaligned_i64);
}

printf("\ndone\n");
return 0;
}
13 changes: 7 additions & 6 deletions test/other/test_parseTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,17 @@ mergeInto(LibraryManager.library, {
{{{ makeSetValue('ptr', '0', 0x12345678AB, 'i64') }}};
_printI64(ptr);

// This value doesn't fit into i64. The current behaviour here is to
// truncate and round (see splitI16 in parseTools.js)
// This value doesn't fit into i64. The current behaviour truncate (i.e.
// ignore the upper bits), in the same way that `BigInt64Array[X] = Y` does.
// (see splitI16 in parseTools.js)
_clearI64(ptr);
{{{ makeSetValue('ptr', '0', 0x1122334455667788AA, 'i64') }}};
_printI64(ptr);

_clearI64(ptr);
{{{ makeSetValue('ptr', '0', -0x1122334455667788AA, 'i64') }}};
_printI64(ptr);

_clearI64(ptr);
{{{ makeSetValue('ptr', '0', 0x12345678AB, 'i53') }}};
_printI64(ptr);
Expand All @@ -128,8 +133,4 @@ mergeInto(LibraryManager.library, {
{{{ makeSetValue('ptr', '0', 0x12345678ab, 'i32') }}};
_printI64(ptr);
},

test_makeSetValue_unaligned: function(ptr) {
{{{ makeSetValue('ptr', '0', 0x12345678AB, 'i64') }}};
},
});
7 changes: 2 additions & 5 deletions test/other/test_parseTools.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ ptr: edcba988

test_makeSetValue:
printI64: 0x12345678ab
printI64: 0xffffffff66780000
printI64: 0x2233445566780000
printI64: 0xddccbbaa99880000
printI64: 0x12345678ab
printI64: 0xffffffffffffffff
printI64: 0xff
Expand Down Expand Up @@ -61,8 +62,4 @@ arg1: -9007199254740992
arg2: 0
rtn = 0

test_makeSetValue_unaligned
i64 = 0x12345678ab
i64 = 0x12345678ab

done
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size.js.size
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
59613
59584
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_no_asserts.js.size
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
33284
33255
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_strict.js.size
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
58555
58526