Bug 159649 – op_add/ValueAdd should be an IC in all JIT tiers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

Some motivation:
on x86-64:
- We emit a fast path that is ~144 bytes in size.
- We emit a slow path that is ~78 bytes in size.
- If we just emitted an int+int fast path, that's 39 bytes.

It seems like there is potential here for making things faster and more memory efficient.

[Saam Barati]

Created attachment 283381 [details]
it begins

[Saam Barati]

Created attachment 283494 [details]
WIP

[Saam Barati]

Created attachment 283594 [details]
WIP

[Saam Barati]

Created attachment 283620 [details]
WIP

[Saam Barati]

Created attachment 283830 [details]
WIP

almost done, just have to make sure 32-bit works, and clean up a few things.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:76:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:26:  #ifndef header guard has wrong style, please use: JITMathIC_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:787:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:62:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JIT.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:55:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8084:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:3052:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3431:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/ArithProfile.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 31 files


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

[Saam Barati]

Created attachment 283975 [details]
WIP

almost done

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:76:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:184:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:26:  #ifndef header guard has wrong style, please use: JITMathIC_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:787:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:62:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JIT.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:3055:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecode/ArithProfile.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/Repatch.h:57:  The parameter name "call" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:57:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 20 in 40 files


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

[Saam Barati]

Created attachment 283982 [details]
WIP

maybe 32-bit works now.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:76:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:184:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:26:  #ifndef header guard has wrong style, please use: JITMathIC_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/jit/JITMathIC.h:81:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:790:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:62:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JIT.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:3055:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:41:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/bytecode/ArithProfile.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/jit/Repatch.h:57:  The parameter name "call" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:57:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 20 in 39 files


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

[Saam Barati]

Created attachment 284034 [details]
WIP

Need to take some data, and write a changelog.
Then I'll remove code size profiling code, and patch should be ready for review.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:177:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:3055:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:57:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 9 in 39 files


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

[Saam Barati]

Created attachment 284077 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 9 in 45 files


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

[Build Bot]

Comment on attachment 284077 [details]
patch

Attachment 284077 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1709885

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 284080 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 284082 [details]
performance

I re-ran kraken a couple times. It's a bit noisy, but seems neutral.

[Saam Barati]

Created attachment 284083 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 9 in 45 files


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

[Benjamin Poulain]

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

Some comments:

> Source/JavaScriptCore/ChangeLog:8
> +        This patch makes Add and IC inside all JIT tiers. It does so in a

and->an?

> Source/JavaScriptCore/ChangeLog:37
> +        Making an arithmetic IC is now easy. The JITMathIC class will hold a snippet

The naming is a bit weird for Add since it is not only an arithmetic operation but also a string operation.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3007
> +    ConcurrentJITLocker locker(m_lock);

Why is this synchronized?

I don't think we ever compile the same codeblock concurrently on different JIT?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3388
> +    ArithProfile* arithProfile = m_jit.graph().baselineCodeBlockFor(node->origin.semantic)->arithProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);

Do you need to get the baseline code block?
Isn't the Instruction stream shared?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2851
> +        std::function<void()> m_generator;
> +        Node* m_currentNode;
> +        unsigned m_streamIndex;

Since those members are public, they should not be prefixed with "m_"

[Saam Barati]

(In reply to comment #20)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284077&action=review
> 
> Some comments:
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        This patch makes Add and IC inside all JIT tiers. It does so in a
> 
> and->an?
Yep. Typo.

> 
> > Source/JavaScriptCore/ChangeLog:37
> > +        Making an arithmetic IC is now easy. The JITMathIC class will hold a snippet
> 
> The naming is a bit weird for Add since it is not only an arithmetic
> operation but also a string operation.
Indeed. This is where "+" being overloaded with string concatenation makes naming this hard. I don't know of a better name. Do you have any suggestions? I chose "math" because every other IC we make will really care about operating over numbers.
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3007
> > +    ConcurrentJITLocker locker(m_lock);
> 
> Why is this synchronized?
> 
> I don't think we ever compile the same codeblock concurrently on different
> JIT?

I just copied what adding a StructureStubInfo was doing. This isn't needed though, because we don't ever iterate over the add ICs. I've removed the locking.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3388
> > +    ArithProfile* arithProfile = m_jit.graph().baselineCodeBlockFor(node->origin.semantic)->arithProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);
> 
> Do you need to get the baseline code block?
> Isn't the Instruction stream shared?
Well, this is just a convenient way to get the CodeBlock corresponding to the current CodeOrigin. We may want to use the ArithProfile of an inlined CodeBlock. We could manually switch on there being an inline call frame or not, and return he not-profiled block, but this doesn't buy us anything, because as you said, they're the same instruction stream. I'm just using this method because it's already implemented and does what I want.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2851
> > +        std::function<void()> m_generator;
> > +        Node* m_currentNode;
> > +        unsigned m_streamIndex;
> 
> Since those members are public, they should not be prefixed with "m_"
Fixed. I also fixed the "m_" prefix for public fields inside MathICGenerationState

[Mark Lam]

Comment on attachment 284083 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:54
> +        paths that are more successful than what I tried implementing, but I think that's worth doing
> +        this in follow up patches once the JITMathIC foundation has landed.

I suggest replacing "thats's worth doing this in follow up patches once ..." to "it's worth deferring this to follow up patches after ...".

> Source/JavaScriptCore/bytecode/ArithProfile.h:27
> +/*
> + * Copyright (C) 2016 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

I think this is the wrong copyright notice to use.  Please see LLIntData.h for an example of the new copyright notice that we should be using.

[Saam Barati]

Created attachment 284150 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:49:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 8 in 45 files


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

[Saam Barati]

Created attachment 284151 [details]
patch

fix include order style

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8176:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 7 in 45 files


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

[Benjamin Poulain]

wip:

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3396
> +        Vector<SilentRegisterSavePlan, 4> savePlans;

Having an inline capacity is a bad thing here since you need to copy or move it in the lambda.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3401
> +        addSlowPathGenerator([=] () {

You should use [=, savePlans = WTFMove(savePlans)].
Otherwise the instantiation of this lambda will take a copy of the vector.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2849
> +        std::function<void()> m_generator;

I think a WTF::Function would be better here. Ideally we should change addSlowPath generator accordingly.

[Mark Lam]

Comment on attachment 284151 [details]
patch

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

> Source/JavaScriptCore/bytecode/ArithProfile.h:73
> +    static const uint32_t rhsResultTypeShift = 5;
> +    static const uint32_t lhsResultTypeShift = 11;
> +    static const uint32_t rhsObservedTypeShift = 17;
> +    static const uint32_t lhsObservedTypeShift = 20;

I feel like you can make this a bit less manual.  e.g.

private:
    enum ObservedResultsBits {
        NonNegZeroDoubleBit,
        NegZeroDoubleBit,
        NonNumber,
        Int32Overflow,
        Int52Overflow,
        NumberOfObservedResultsBits
    };

// Later below in the code ...
public:
    enum ObservedResults {
        NonNegZeroDouble = 1 << NonNegZeroDoubleBit,
        NegZeroDouble    = 1 << NegZeroDoubleBit,
        NonNumber        = 1 << NonNumber,
        Int32Overflow    = 1 << Int32Overflow,
        Int52Overflow    = 1 << Int52Overflow,
    };

The down side of the above is that the list is defined twice.  You could just keep the ObservedResults enum list the way you had it, and just add a:
    static const uint32_t numberOfObservedResultsBits = 5;
... just below the enum list.  That way it is tracked together.  The reason we want the numberOfObservedResultsBits is ...

private:
    static const uint32_t numberOfFlagBits = NumberOfObservedResultsBits;
    static const uint32_t rhsResultTypeShift = numberOfFlagBits;
    static const uint32_t lhsResultTypeShift = rhsResultTypeShift + ResultType::numBitsNeeded;
    static const uint32_t rhsObservedTypeShift = lhsResultTypeShift + ResultType::numBitsNeeded;
    static const uint32_t lhsObservedTypeShift = rhsObservedTypeShift + ObservedType:: numBitsNeeded; // And we add a numBitsNeeded in ObservedType.
    static_assert(lhsObservedTypeShift + ObservedType:: numBitsNeeded <= sizeof(uint32_t), "reasons ...");

This way, if we add a bit to ObservedType or ObservedResultsBits, the shift values will update appropriately.

That said, it is harder to read the code this way and it is more rare to add a bit then to have to read this code.  So, alternatively, you can keep the shift values declared the way you had it and add some static asserts:

    static_assert(numberOfFlagBits == NumberOfObservedResultsBits, "reasons ...");
    static_assert(rhsResultTypeShift == numberOfFlagBits, "reasons ...");
    static_assert(lhsResultTypeShift == rhsResultTypeShift + ResultType::numBitsNeeded, "reasons ...");
    static_assert(rhsObservedTypeShift == lhsResultTypeShift + ResultType::numBitsNeeded, "reasons ...");
    static_assert(lhsObservedTypeShift == rhsObservedTypeShift + ObservedType:: numBitsNeeded, "reasons ...");
    static_assert(lhsObservedTypeShift + ObservedType:: numBitsNeeded <= sizeof(uint32_t), "reasons ...");

This way, if someone gets the manually entered values wrong, we will at least catch it right away.  Using a manually entered ObservedType::numberOfBits and numberOfObservedResultsBits is not ideal, but it still lessens the chance of error if we keep those values near where their respective bits are defined.

> Source/JavaScriptCore/bytecode/ArithProfile.h:77
> +    static const uint32_t bottom6Bits = (1 << 6) - 1;
> +    static const uint32_t bottom3Bits = (1 << 3) - 1;
> +    static const uint32_t clearRhsObservedTypeBits = ~((1 << 17) | (1 << 18) | (1 << 19));
> +    static const uint32_t clearLhsObservedTypeBits = ~((1 << 20) | (1 << 21) | (1 << 22));

I think we typically name these as "Mask"s.  Hence, let's call these bottom6BitsMask, bottom3BitsMask, rhsObservedTypeBitsMask, and lhsObservedTypeBitsMask respectively.

Actually, since bottom6BitsMask is exclusively used for masking ResultType bits and bottom3Bits is exclusively for masking ObservedType type bits, perhaps it would be better to name and declare these as follows:
    static const uint32_t resultTypeMask = (1 << ResultType::numBitsNeeded) - 1;
    static const uint32_t observedTypeMask = (1 << ObservedType:: numBitsNeeded) - 1;

> Source/JavaScriptCore/bytecode/ArithProfile.h:92
> +    ArithProfile()
> +    { }

I think for empty functions like this, you can put the { } on the same line.  The only time we put it on the a different line is if there's a constructor initialization list here.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4381
> +    Instruction* instruction = &instructions()[bytecodeOffset + 4];

Will the offset always be 4?  Can we at least give it a symbolic name?  I think that that will help us find all uses of this constant later (if needed).  I think the value is 4 because op_add is a binary op, and the 3 preceding operands are for the result, lhsOp, and rhsOp.  So, maybe we can call this constant the binaryOpArithProfileOffset.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4404
> +    return *bitwise_cast<ArithProfile*>(&pc[4].u.operand);

Ditto.  Use the binaryOpArithProfileOffset symbollic constant here.

In addition, can you change arithProfileForBytecodeOffset() to call arithProfileForPC() instead?  That way, the switch on opcodeID logic need not be duplicated.

[Benjamin Poulain]

Comment on attachment 284151 [details]
patch

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

I could not find any bug, r=me.

I am a bit concerned with the amount of duplicated logic. No idea how to fix it with DFG and FTL being so different :(

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8154
> +    void emitAddSnippet()

Why not just put this code in compileValueAdd()?

> Source/JavaScriptCore/jit/JITMathIC.h:92
> +        // because if we fail allocating the out of line path, we don't want to waist time trying to

waist->waste

[Joseph Pecoraro]

Comment on attachment 284151 [details]
patch

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

> Source/JavaScriptCore/CMakeLists.txt:180
> +    bytecode/ArithProfile.cpp
>      bytecode/ArrayAllocationProfile.cpp
>      bytecode/ArrayProfile.cpp

This is already in the list.

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:96
> +        if (profile.didObserveNegZeroDouble()) {
> +            out.print(separator, "NegZeroDouble");
> +            separator = "|";
> +        }
> +        if (profile.didObserveNonNegZeroDouble()) {
> +            out.print("NonNegZeroDouble");
> +            separator = "|";
> +        }

`separator` is only used in that one place and nowhere else. Should all of these out.print(...) be out.print(separator, ...)? Same for ValueProfile.cpp, or am I missing how it gets used?

[Mark Lam]

Comment on attachment 284151 [details]
patch

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

>> Source/JavaScriptCore/CMakeLists.txt:180
>>      bytecode/ArrayProfile.cpp
> 
> This is already in the list.

Nope.  One is "Arith", the other "Array".

>> Source/JavaScriptCore/bytecode/ArithProfile.cpp:96
>> +        }
> 
> `separator` is only used in that one place and nowhere else. Should all of these out.print(...) be out.print(separator, ...)? Same for ValueProfile.cpp, or am I missing how it gets used?

Joe, is right.  These should all be out.print(separator, ...).

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:42
> +    if (m_arithProfile) {

Is there any reason why there won't be an available arithProfile anymore?  I know with the ResultProfile, this was conditional.  But now, it's embedded in the bytecode stream.  Should this be unconditional?

I see that in all JIT tiers, m_arithProfile is a non-null value.  The only time it is null is if we build with the DFG disabled.  We used to pass null for the resultProfile pointer because profiling doesn't make sense back then when the DFG is disabled.  But with your IC also feeding off the profiling data, I think it no longer make sense to ever pass a null arithProfile.  So, I think you can make this code unconditional.

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:129
> +

Please remove this extra line.

> Source/JavaScriptCore/jit/JITAddGenerator.cpp:171
> +    if (m_arithProfile)
> +        m_arithProfile->emitSetDouble(jit);

I see that that all JIT tiers now pass in a non-null arithProfile.  Does it make sense to write profiling info like this in the DFG and FTL tiers?  If not, you should change the snippet to pass in a bool to indicate whether to do this profiling or not, instead of relying on the null-ness of the m_arithProfile pointer (since you will want it to be non-null so that you read the collected profiling info in the generateInline().

> Source/JavaScriptCore/jit/JITAddGenerator.h:43
> +    JITAddGenerator()
> +    { }

Can put { } on the same line.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:682
> +    return OperandTypes(ArithProfile::fromInt(instruction[4].u.operand).lhsResultType(), ArithProfile::fromInt(instruction[4].u.operand).rhsResultType());

I think you should express this as:
    ArithProfile profile = ArithProfile::fromInt(instruction[4].u.operand);
    return OperandTypes(profile.lhsResultType(), profile.rhsResultType());

It makes it easier to see at a glance that the lhs and rhs came from the same ArithProfile.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:692
> +    OperandTypes types = getOperandTypes(copiedInstruction(currentInstruction));

Why not get rid of this marshaling into OperandTypes and use the ArithProfile itself e.g.
    ArithProfile arithProfile = ArithProfile::fromInt(instruction[4].u.operand);

... and down below, just get the lhs and rhs operand type directly from the arithProfile when instantiating the SnippetOperands.  With that, you can get rid of the getOperandTypes() helper function too.  Would that work?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:699
> +    OperandTypes types = getOperandTypes(currentInstruction);

Ditto.  No need for this?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:828
>      SnippetOperand leftOperand(types.first());
>      SnippetOperand rightOperand(types.second());

Use the arithProfile's lhs and rhs to initialize the SnippetOperands, and get rid of OperandTypes and getOperandTypes()?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:903
>      SnippetOperand leftOperand(types.first());
>      SnippetOperand rightOperand(types.second());

Use the arithProfile's lhs and rhs to initialize the SnippetOperands, and get rid of OperandTypes and getOperandTypes()?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:953
> +    OperandTypes types = getOperandTypes(copiedInstruction(currentInstruction));

Use the arithProfile's lhs and rhs to initialize the SnippetOperands, and get rid of OperandTypes and getOperandTypes()?

> Source/JavaScriptCore/jit/JITArithmetic.cpp:1013
>      SnippetOperand leftOperand(types.first());
>      SnippetOperand rightOperand(types.second());

Use the arithProfile's lhs and rhs to initialize the SnippetOperands, and get rid of OperandTypes and getOperandTypes()?

> Source/JavaScriptCore/jit/JITMathIC.h:47
> +    bool shouldSlowPathRepatch;

"shouldSlowPathRepatch" sounds strange.  Did you mean "shouldDoSlowPathRepatch"?

> Source/JavaScriptCore/jit/JITOperations.cpp:2274
> +    arithProfile->detectNumericness(result);

Suggestion: maybe you should rename "detectNumericness" as "observeResult" to match "observeLHSAndRHS".  Previously, we're working with the ResultProfile where detectNumericness implies numericness of the result.  But the ArithProfile profiles both the operands and the result.  Hence, I think we should make it explicit that we're observing the result here.

> Source/JavaScriptCore/jit/JITOperations.cpp:2331
> +EncodedJSValue JIT_OPERATION operationValueAddNonOptimize(ExecState* exec, EncodedJSValue encodedOp1, EncodedJSValue encodedOp2, JITAddIC*)

I see that above, you have a operationValueAddNotProfiledNotOptimize, but here you called it "...NonOptimize".  I think you should be consistent.

Suggestion: By the "Not" or "Non", I think you are trying to describe what the operation's extra actions (or lack thereof).  Would you consider these alternatives:
    1. "Profiling"/"NoProfiling" and "Optimizing"/"NoOptimizing" (describes what extra work the operation includes or not)
    2. "Profile"/"DontProfile" and "Optimize"/"DontOptimize" (describes what extra work the operation should do or not)

For example, operationValueAddNotProfiledNotOptimize would become operationValueAddWithNoProfilingAndNoOptimizing.

IMHO, "Non" and "Not" sounds like they belong with a state (e.g. NonProfiled, NonOptimized), and not so much an action.

[Saam Barati]

(In reply to comment #30)
> Comment on attachment 284151 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284151&action=review
> 
> > Source/JavaScriptCore/CMakeLists.txt:180
> > +    bytecode/ArithProfile.cpp
> >      bytecode/ArrayAllocationProfile.cpp
> >      bytecode/ArrayProfile.cpp
> 
> This is already in the list.
> 
> > Source/JavaScriptCore/bytecode/ArithProfile.cpp:96
> > +        if (profile.didObserveNegZeroDouble()) {
> > +            out.print(separator, "NegZeroDouble");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveNonNegZeroDouble()) {
> > +            out.print("NonNegZeroDouble");
> > +            separator = "|";
> > +        }
> 
> `separator` is only used in that one place and nowhere else. Should all of
> these out.print(...) be out.print(separator, ...)? Same for
> ValueProfile.cpp, or am I missing how it gets used?
I'm going to remove this. I copied this method from the old ResultProfile. It's currently not used, and it's also wrong. I'm going to remove it's implementation and we can implement a correct version if we ever want to use it.

[Saam Barati]

Comment on attachment 284151 [details]
patch

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

>>>> Source/JavaScriptCore/CMakeLists.txt:180
>>>>      bytecode/ArrayProfile.cpp
>>> 
>>> This is already in the list.
>> 
>> Nope.  One is "Arith", the other "Array".
> 
> I'm going to remove this. I copied this method from the old ResultProfile. It's currently not used, and it's also wrong. I'm going to remove it's implementation and we can implement a correct version if we ever want to use it.

I'm wrong. It is used. I'll write one that works.

>> Source/JavaScriptCore/bytecode/ArithProfile.h:73
>> +    static const uint32_t lhsObservedTypeShift = 20;
> 
> I feel like you can make this a bit less manual.  e.g.
> 
> private:
>     enum ObservedResultsBits {
>         NonNegZeroDoubleBit,
>         NegZeroDoubleBit,
>         NonNumber,
>         Int32Overflow,
>         Int52Overflow,
>         NumberOfObservedResultsBits
>     };
> 
> // Later below in the code ...
> public:
>     enum ObservedResults {
>         NonNegZeroDouble = 1 << NonNegZeroDoubleBit,
>         NegZeroDouble    = 1 << NegZeroDoubleBit,
>         NonNumber        = 1 << NonNumber,
>         Int32Overflow    = 1 << Int32Overflow,
>         Int52Overflow    = 1 << Int52Overflow,
>     };
> 
> The down side of the above is that the list is defined twice.  You could just keep the ObservedResults enum list the way you had it, and just add a:
>     static const uint32_t numberOfObservedResultsBits = 5;
> ... just below the enum list.  That way it is tracked together.  The reason we want the numberOfObservedResultsBits is ...
> 
> private:
>     static const uint32_t numberOfFlagBits = NumberOfObservedResultsBits;
>     static const uint32_t rhsResultTypeShift = numberOfFlagBits;
>     static const uint32_t lhsResultTypeShift = rhsResultTypeShift + ResultType::numBitsNeeded;
>     static const uint32_t rhsObservedTypeShift = lhsResultTypeShift + ResultType::numBitsNeeded;
>     static const uint32_t lhsObservedTypeShift = rhsObservedTypeShift + ObservedType:: numBitsNeeded; // And we add a numBitsNeeded in ObservedType.
>     static_assert(lhsObservedTypeShift + ObservedType:: numBitsNeeded <= sizeof(uint32_t), "reasons ...");
> 
> This way, if we add a bit to ObservedType or ObservedResultsBits, the shift values will update appropriately.
> 
> That said, it is harder to read the code this way and it is more rare to add a bit then to have to read this code.  So, alternatively, you can keep the shift values declared the way you had it and add some static asserts:
> 
>     static_assert(numberOfFlagBits == NumberOfObservedResultsBits, "reasons ...");
>     static_assert(rhsResultTypeShift == numberOfFlagBits, "reasons ...");
>     static_assert(lhsResultTypeShift == rhsResultTypeShift + ResultType::numBitsNeeded, "reasons ...");
>     static_assert(rhsObservedTypeShift == lhsResultTypeShift + ResultType::numBitsNeeded, "reasons ...");
>     static_assert(lhsObservedTypeShift == rhsObservedTypeShift + ObservedType:: numBitsNeeded, "reasons ...");
>     static_assert(lhsObservedTypeShift + ObservedType:: numBitsNeeded <= sizeof(uint32_t), "reasons ...");
> 
> This way, if someone gets the manually entered values wrong, we will at least catch it right away.  Using a manually entered ObservedType::numberOfBits and numberOfObservedResultsBits is not ideal, but it still lessens the chance of error if we keep those values near where their respective bits are defined.

I'm going to go with a variant of your first suggestion.
Basically, I'll make it that you don't have to change anything if you change the bits required, except the code for code for clearRhsObservedTypeBits/clearLhsObservedTypeBits. Those will make a hard assumption that ObservedType is 3 bits, with a static assert.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4381
>> +    Instruction* instruction = &instructions()[bytecodeOffset + 4];
> 
> Will the offset always be 4?  Can we at least give it a symbolic name?  I think that that will help us find all uses of this constant later (if needed).  I think the value is 4 because op_add is a binary op, and the 3 preceding operands are for the result, lhsOp, and rhsOp.  So, maybe we can call this constant the binaryOpArithProfileOffset.

I don't think this will actually help. We have code like this all over the place, and we don't give offsets symbolic names.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4404
>> +    return *bitwise_cast<ArithProfile*>(&pc[4].u.operand);
> 
> Ditto.  Use the binaryOpArithProfileOffset symbollic constant here.
> 
> In addition, can you change arithProfileForBytecodeOffset() to call arithProfileForPC() instead?  That way, the switch on opcodeID logic need not be duplicated.

Yeah, I'll do that. The logic does have to be duplicated though. One function returns nullptr based on the switch, and the other asserts it is a proper bytecode instruction.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8154
>> +    void emitAddSnippet()
> 
> Why not just put this code in compileValueAdd()?

Done.

>> Source/JavaScriptCore/jit/JITAddGenerator.cpp:42
>> +    if (m_arithProfile) {
> 
> Is there any reason why there won't be an available arithProfile anymore?  I know with the ResultProfile, this was conditional.  But now, it's embedded in the bytecode stream.  Should this be unconditional?
> 
> I see that in all JIT tiers, m_arithProfile is a non-null value.  The only time it is null is if we build with the DFG disabled.  We used to pass null for the resultProfile pointer because profiling doesn't make sense back then when the DFG is disabled.  But with your IC also feeding off the profiling data, I think it no longer make sense to ever pass a null arithProfile.  So, I think you can make this code unconditional.

This isn't always true. Sometimes, the baseline will not emit profiling, in which case this is nullptr.
It's true that the higher tiers won't pass in nullptr. We can refine this later on.
W.r.t emitting the profiling bit setting, we only do that now if the bits weren't set previously. We can refine this in follow up patches to be tier-aware.

>> Source/JavaScriptCore/jit/JITArithmetic.cpp:828
>>      SnippetOperand rightOperand(types.second());
> 
> Use the arithProfile's lhs and rhs to initialize the SnippetOperands, and get rid of OperandTypes and getOperandTypes()?

I wrote this out, but it doesn't actually simplify the code. We need to either choose a name for ArithProfile that doesn't conflict with the arithProfile below. Or, we need to assign variable names to the lhs/rhs ResultType. All of these require more or equal lines of code to what we have now.
getOperandTypes() is also helpful for abstracting over 32/64 bit.

>> Source/JavaScriptCore/jit/JITMathIC.h:47
>> +    bool shouldSlowPathRepatch;
> 
> "shouldSlowPathRepatch" sounds strange.  Did you mean "shouldDoSlowPathRepatch"?

I don't agree. shouldDoSlowPathRepatch sounds much worse to me.

[Saam Barati]

(In reply to comment #27)
> wip:
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284083&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3396
> > +        Vector<SilentRegisterSavePlan, 4> savePlans;
> 
> Having an inline capacity is a bad thing here since you need to copy or move
> it in the lambda.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3401
> > +        addSlowPathGenerator([=] () {
> 
> You should use [=, savePlans = WTFMove(savePlans)].
> Otherwise the instantiation of this lambda will take a copy of the vector.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2849
> > +        std::function<void()> m_generator;
> 
> I think a WTF::Function would be better here. Ideally we should change
> addSlowPath generator accordingly.
I'm going to make this change in another patch since this is just a refactoring.
https://bugs.webkit.org/show_bug.cgi?id=160055

[Saam Barati]

Created attachment 284273 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:169:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1562:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 7 in 45 files


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

[Saam Barati]

Created attachment 284277 [details]
patch for landing

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.cpp:169:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITSubGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1562:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITMulGenerator.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:367:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:58:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 7 in 45 files


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

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/203537

[Csaba Osztrogonác]

(In reply to comment #39)
> landed in:
> https://trac.webkit.org/changeset/203537

- ARM Linux buildfix landed in https://trac.webkit.org/changeset/203595
- It broke the Windows 64-bit build too, see bug160080 for details

[Csaba Osztrogonác]

and it made many tests crash on ARMv7 Linux platforms:
https://bugs.webkit.org/show_bug.cgi?id=160082

[Csaba Osztrogonác]

(In reply to comment #41)
> and it made many tests crash on ARMv7 Linux platforms:
> https://bugs.webkit.org/show_bug.cgi?id=160082

bug report to track ARM traditional crashes: bug160157