Bug 170909 – WebAssembly: fast memory cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

Address a few follow-up comments from Saam in https://bugs.webkit.org/show_bug.cgi?id=170628

[JF Bastien]

Created attachment 307285 [details]
patch

[Build Bot]

Attachment 307285 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 307285 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307285&action=review

> Source/JavaScriptCore/b3/B3WasmBoundsCheckValue.h:63
> +    JS_EXPORT_PRIVATE WasmBoundsCheckValue(Origin, Value* ptr, GPRReg pinnedGPR, unsigned offset, size_t maximum = std::numeric_limits<uint32_t>::max());

IMO, this logic for the maximum does not belong in B3. B3 should not know about the semantics of Wasm memory ops. I think you should provide the UINT_MAX from WasmB3IRGenerator. If anything, you could create two constructors to this node, one where the register is pinned, and one where it's not. Then WasmB3IRGenerator should pass in UINT_MAX. Also, I think your comments that describes the semantics in B3LowerToAir should go into WasmB3IRGenerator.

[JF Bastien]

Created attachment 307493 [details]
patch

Address Saam's comments.

[Build Bot]

Attachment 307493 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WasmMemory.cpp:327:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 307493 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307493&action=review

r=me

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3173
> +                ASSERT(value->bounds().maximum <= value->redzoneLimit());

Nit: It seems weird that B3/Air are asserting this, and that this is a function. I would just assert this when making the node.

> Source/JavaScriptCore/b3/B3Validate.cpp:478
> +                    VALIDATE(value->as<WasmBoundsCheckValue>()->bounds().maximum <= value->as<WasmBoundsCheckValue>()->redzoneLimit(), ("At ", *value));

ditto

[JF Bastien]

Created attachment 307506 [details]
patch

Address Saam's comments.

[WebKit Commit Bot]

Comment on attachment 307506 [details]
patch

Clearing flags on attachment: 307506

Committed r215533: <http://trac.webkit.org/changeset/215533>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.