1464790 - js/src/wasm/WasmTypes.h: define MaxMemoryAccessSize correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Julian Seward [:jseward]]

I was studying the bounds-check stuff some more and noticed that
we have MaxMemoryAccessSize = sizeof(Val).  And I was wondering
if that is really intended, since it includes not only the size of the
largest underlying value, but also the size of the tag (Val::type_) field.

Indeed, printing MaxMemoryAccessSize gets me 24 on x86_64 and 20 on x86.

It's not harmful, since it overstates the size of the largest access
(16, unless we're doing x86 YMM loads/stores), but IIUC it is
misleading.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1464790-fix-MaxMemoryAccessSize-1.diff

A possible fix.  Unfortunately the obvious scheme of using 'sizeof(Val::u)'
doesn't work because |u| is a private field.  The solution of extracting
the size with a constexpr method is the least-worst think I could think of.
Better solutions welcomed.

Comment 2

[Lars T Hansen [:lth]]

Comment on attachment 8981122 [details] [diff] [review]
bug1464790-fix-MaxMemoryAccessSize-1.diff

Review of attachment 8981122 [details] [diff] [review]:
-----------------------------------------------------------------

You should ask Luke for review.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8981122 [details] [diff] [review]
bug1464790-fix-MaxMemoryAccessSize-1.diff

Review of attachment 8981122 [details] [diff] [review]:
-----------------------------------------------------------------

Ah right, I suppose it is overly-conservative which can cause confusion.

::: js/src/wasm/WasmTypes.h
@@ +1866,5 @@
> +// AVX-512 loads/stores, hence the upper limit of 64.
> +static_assert(MaxMemoryAccessSize >= 8,  "MaxMemoryAccessSize too low");
> +static_assert(MaxMemoryAccessSize <= 64, "MaxMemoryAccessSize too high");
> +static_assert((MaxMemoryAccessSize & (MaxMemoryAccessSize-1)) == 0,
> +              "MaxMemoryAccessSize is not a power of two");

Usually we statically assert something because the correctness of some other piece of code depends on it (in which case we comment what that code is so we can go fix it later if need be), but I can't think of how we depend on any of these 3 conditions holding.  If you can, that might be the appropriate comment for the static asserts; otherwise, if you think they are valuable as sanity checks, maybe just stick it in WasmTypes.cpp (to avoid cluttering the header) and comment that they are just sanity asserts.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Is there anything blocking landing here?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d8e05af3d9
js/src/wasm/WasmTypes.h: define MaxMemoryAccessSize correctly.  r=luke.

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/24d8e05af3d9