Bug 170909 - WebAssembly: fast memory cleanups
Summary: WebAssembly: fast memory cleanups
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 170628
Blocks: 159775
  Show dependency treegraph
 
Reported: 2017-04-17 11:25 PDT by JF Bastien
Modified: 2017-04-19 15:05 PDT (History)
8 users (show)

See Also:


Attachments
patch (15.41 KB, patch)
2017-04-17 11:30 PDT, JF Bastien
no flags Details | Formatted Diff | Diff
patch (24.06 KB, patch)
2017-04-19 11:29 PDT, JF Bastien
sbarati: review+
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (24.32 KB, patch)
2017-04-19 13:58 PDT, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2017-04-17 11:25:08 PDT
Address a few follow-up comments from Saam in https://bugs.webkit.org/show_bug.cgi?id=170628
Comment 1 JF Bastien 2017-04-17 11:30:58 PDT
Created attachment 307285 [details]
patch
Comment 2 Build Bot 2017-04-17 11:33:35 PDT
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.
Comment 3 Saam Barati 2017-04-17 11:42:12 PDT
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.
Comment 4 JF Bastien 2017-04-19 11:29:02 PDT
Created attachment 307493 [details]
patch

Address Saam's comments.
Comment 5 Build Bot 2017-04-19 11:31:19 PDT
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.
Comment 6 Saam Barati 2017-04-19 12:44:32 PDT
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
Comment 7 JF Bastien 2017-04-19 13:58:41 PDT
Created attachment 307506 [details]
patch

Address Saam's comments.
Comment 8 WebKit Commit Bot 2017-04-19 15:05:54 PDT
Comment on attachment 307506 [details]
patch

Clearing flags on attachment: 307506

Committed r215533: <http://trac.webkit.org/changeset/215533>
Comment 9 WebKit Commit Bot 2017-04-19 15:05:56 PDT
All reviewed patches have been landed.  Closing bug.