Bug 161471 - WASM functions should be able to use arguments
Summary: WASM functions should be able to use arguments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-31 20:13 PDT by Keith Miller
Modified: 2016-09-07 12:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (58.81 KB, patch)
2016-08-31 20:46 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (55.20 KB, patch)
2016-08-31 20:57 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (57.43 KB, patch)
2016-08-31 21:07 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (28.46 KB, patch)
2016-09-01 14:35 PDT, Keith Miller
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-08-31 20:13:43 PDT
WASM functions should be able to use arguments
Comment 1 Keith Miller 2016-08-31 20:46:07 PDT
Created attachment 287591 [details]
Patch
Comment 2 Keith Miller 2016-08-31 20:57:07 PDT
Created attachment 287593 [details]
Patch
Comment 3 WebKit Commit Bot 2016-08-31 21:00:07 PDT
Attachment 287593 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/testWASM.cpp:335:  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: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Keith Miller 2016-08-31 21:02:50 PDT
> ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  Tests for true/false,
> null/non-null, and zero/non-zero should all be done without equality
> comparisons.  [readability/comparison_to_zero] [5]
> ERROR: Source/JavaScriptCore/testWASM.cpp:335:  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: 2 in 21 files

While these are technically violations of WebKit style I think they are warranted in those spots. Each case is surrounded by other checks that the result of similar expressions are equal to non-zero values. Using ! instead just makes the code harder to read.
Comment 5 Keith Miller 2016-08-31 21:07:03 PDT
Created attachment 287594 [details]
Patch
Comment 6 WebKit Commit Bot 2016-08-31 21:08:56 PDT
Attachment 287594 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/wasm/WASMFormat.h:63:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/testWASM.cpp:335:  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: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Keith Miller 2016-09-01 10:31:38 PDT
Comment on attachment 287594 [details]
Patch

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

> Source/JavaScriptCore/B3CallingConventions.cpp:29
> +

There should be a `#if ENABLE(B3_JIT)` here. That fixes the 32-bit build. I'll make sure to change that before landing.
Comment 8 Keith Miller 2016-09-01 14:35:05 PDT
Created attachment 287681 [details]
Patch
Comment 9 WebKit Commit Bot 2016-09-01 14:37:34 PDT
Attachment 287681 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/testWASM.cpp:335:  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 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Mark Lam 2016-09-01 16:06:23 PDT
Comment on attachment 287681 [details]
Patch

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

> Source/JavaScriptCore/testWASM.cpp:196
> +// FIXME: This is pure hacks and should be changed... please...

If this is still true, you need a bug url to go with this.
Comment 11 Keith Miller 2016-09-01 16:52:29 PDT
Comment on attachment 287681 [details]
Patch

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

>> Source/JavaScriptCore/testWASM.cpp:196
>> +// FIXME: This is pure hacks and should be changed... please...
> 
> If this is still true, you need a bug url to go with this.

I don't think that's an issue. All this code is just here until the actual testing infrastructure is in place. I'll change the comment to exclude a FIXME.
Comment 12 Benjamin Poulain 2016-09-01 17:06:34 PDT
Comment on attachment 287681 [details]
Patch

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

> Source/JavaScriptCore/B3CallingConventions.cpp:35
> +static JSCCallingConvention* staticJSCCallingConvention;

This should be inside jscCallingConvention.

Personally I find it nicer to explicitly initialize it.

> Source/JavaScriptCore/B3CallingConventions.cpp:41
> +    staticJSCCallingConvention = new CallingConvention<jscHeaderSize, nextJSCOffset>(Vector<GPRReg>(), RegisterSet::calleeSaveRegisters());

You could use NeverDestroyed to avoid locking a heap page.

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:107
> +    m_locals.grow(count);

I don't think that's what you want.
This change the size of m_locals TO count, and default initialize the value.

If count > m_locals.size(), m_locals.append() will go after that.

What you want is reserveCapacity(size() + count)

> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:127
> +    ASSERT(index < m_locals.size());

This should not be needed. Vector does that unless you tell it not to.

What could be valuable is 
    ASSERT(m_locals[index])
to make sure the value is not null.
Comment 13 Mark Lam 2016-09-01 19:15:02 PDT
Keith landed this patch in https://trac.webkit.org/changeset/205330.  

FYI, there was a ChangeLog mixup.  So, don't just believe what you read as the change on trac.  Check the ChangeLog diff itself.