Make makeSetValue rounding consistent with HEAP64 semantics #19239

sbc100 · 2023-04-24T19:21:32Z

This change does two things:

Require 8 byte alignment for makeSetValue of i64. Natural alignment is already required for all other types in makeSetValue and I don't know of good reason not to require this also for i64. The comment here implied that perhaps i64 was not always 8 bytes aligned, perhaps in the fastcomp days. But with wasm32 and wasm64 i64 should always be 8 bytes aligned. Folks with unaligned data already cannot use makeGetValue/makeSetValue.
Make splitI64 consistent with HEAP64 in the way it handles numbers that are outside the i64 range. i.e. truncation

Without the second part of the change the other.test_parseTools_bigint will fail because it will use HEAP64.set to set the value rather than splitI64, and HEAP64.set writes 0x2233445566780000 rather than 0xffffffff66780000.

The existing behaviour of splitI64 to write 0xffffffff66780000 seems rather odd since it seems to use saturation, but only for the upper half.

sbc100 · 2023-04-24T19:34:25Z

The test code for unaligned i64 writes was added in #17619... which seems to suggest that i64 can be unaligned.. but that is not the case AFAICT.

sbc100 · 2023-04-24T19:38:13Z

I verified that I verified that browser.test_wasmfs_fetch_backend_bigint still passes after this change... @kripken do you remember why you though i64 could be unaligned back in #17619?

kripken · 2023-04-24T20:17:15Z

Reading that old PR, it sounds like that fix was necessary and the test I added failed before it. If you go back to the commit before the PR landed and just add the test but not the fix, does it fail?

Specifically it sounds like that test was actually showing an unaligned i64, so understanding how that occurred might be helpful (maybe LLVM doesn't always align them?)

sbc100 · 2023-04-24T20:18:57Z

Reading that old PR, it sounds like that fix was necessary and the test I added failed before it. If you go back to the commit before the PR landed and just add the test but not the fix, does it fail?

Specifically it sounds like that test was actually showing an unaligned i64, so understanding how that occurred might be helpful (maybe LLVM doesn't always align them?)

I verified that the test in question (browser.test_wasmfs_fetch_backend_bigint) passes both before and after this PR. I will see if I can do back to the old version..

sbc100 · 2023-04-24T21:05:55Z

I tracked down the issue. The problem was in proxied_async_js_impl_backend.h:

size_t getSize() override {                                                    
    size_t result;                                                                                                
    proxy([&](auto ctx) {                                                        
      _wasmfs_jsimpl_async_get_size(                                             
        ctx.ctx, getBackendIndex(), getFileIndex(), &result);                    
    });                                                                          
    return result;                                                               
  }

Here size_t is used which is not a 64-bit type, but a pointer-width type. So it was inconsistent with the JS size which in which _wasmfs_jsimpl_async_get_size was using {{{ makeSetValue('size_p', 0, 'size', 'i64') }}};.

getSize was fixed to use off_t instead in #17783.

This change does two things: 1. Require 8 byte alignment for makeSetValue of i64. Natural alignment is already required for all other types in `makeSetValue` and I don't know of good reason not to require this also for i64. The comment here implied that perhaps i64 was not always 8 bytes aligned, perhaps in the fastcomp days. But with wasm32 and wasm64 i64 should always be 8 bytes aligned. Folks with unaligned data already cannot use makeGetValue/makeSetValue. 2. Make splitI64 consistent with assignment to HEAPI64 in the way it handles numbers that are larger than the maximum i64 value.

kripken · 2023-04-24T23:51:14Z

Good find!

kripken

Sorry, I wrote that other comment when I was still reading the code. I have two typos and a question.

kripken · 2023-04-24T23:51:44Z

src/parseTools.js

-    // 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


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

kripken · 2023-04-24T23:54:35Z

src/parseTools.js

  //  $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,


Suggested change

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

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

kripken · 2023-04-24T23:58:40Z

src/parseTools.js

-        asmEnsureFloat(4294967296, 'double') + ')', 'double') + ', ' +
-        asmEnsureFloat(4294967295, 'double') + ')', 'i32') + '>>>0' +
+        asmCoercion('Math.floor((VALUE)/' +
+        asmEnsureFloat(4294967296, 'double') + ')', 'double') + '>>>0' +


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).

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

"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++?

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?

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.

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...

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 >

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?

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.

I see. Thanks, that all makes sense then!

sbc100 · 2023-04-25T00:00:18Z

Oops, sorry I was too quick to land there.

sbc100 requested review from juj and kripken April 24, 2023 19:29

sbc100 force-pushed the makeSetValue_i64 branch 2 times, most recently from ba2527d to e9f917a Compare April 24, 2023 20:08

sbc100 force-pushed the makeSetValue_i64 branch from e9f917a to 5baa86d Compare April 24, 2023 21:12

sbc100 changed the title ~~Make makeSetValue rounding consistent with HEAPI64.set()~~ Make makeSetValue rounding consistent with HEAP64 semantics Apr 24, 2023

sbc100 merged commit e2918ae into main Apr 24, 2023

sbc100 deleted the makeSetValue_i64 branch April 24, 2023 23:53

kripken reviewed Apr 24, 2023

View reviewed changes

sbc100 added a commit that referenced this pull request Apr 25, 2023

Feedback from #19239. NFC

768ca7b

sbc100 mentioned this pull request Apr 25, 2023

Add debug range checks for makeSetValue #19251

Merged

	// 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

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

Make makeSetValue rounding consistent with HEAP64 semantics #19239

Make makeSetValue rounding consistent with HEAP64 semantics #19239

Uh oh!

Conversation

sbc100 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 commented Apr 24, 2023

Uh oh!

sbc100 commented Apr 24, 2023

Uh oh!

kripken commented Apr 24, 2023

Uh oh!

sbc100 commented Apr 24, 2023

Uh oh!

sbc100 commented Apr 24, 2023

Uh oh!

kripken commented Apr 24, 2023

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sbc100 commented Apr 24, 2023 •

edited

Loading

kripken Apr 25, 2023 •

edited

Loading