Bug 160012 – [DFG] Support ArithPow(Untyped, Untyped)

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Yusuke Suzuki]

Now, ** operator is supported. So it is consistent that we support ArithPow(Untyped, Untyped), as similar as ArithSub(Untyped, Untyped), ArithMul(Untyped, Untyped) etc.

[Caio Lima]

Created attachment 284465 [details]
WIP Patch

RFC in this implementation.

[Saam Barati]

Comment on attachment 284465 [details]
WIP Patch

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

You're missing changes to Clobberize rules. Otherwise, I think it LGTM. I can look more closely as this gets updated to be an IC.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4999
> +    if (node->binaryUseKind() == UntypedUse) {

You should wait until my Mul IC patch lands to rebase this, and then this can easily follow the new templates I'm defining for being a Math IC.
Otherwise, LGTM.

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:174
> +                } else if (yOperandValue == 2 && m_node->isBinaryUseKind(UntypedUse)) {

Why is this necessary?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2124
> +        } else {
> +            emitBinarySnippet<JITPowGenerator>(operationValuePow);

Ditto about waiting for the Mul IC to land, and then this can become an IC.
Also, in WebKit, we don't use "{}" around single line body if/else/for/while statements

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:2
> + * Copyright (C) 2015-2016 Apple Inc. All rights reserved.

Should just be 2016.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:36
> +void JITPowGenerator::generateFastPath(CCallHelpers& jit)

I'll read this later. I'll trust that it's probably correct and derived from other code that emits pow code.

> Source/JavaScriptCore/jit/JITPowGenerator.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

should be 2016.

> Source/JavaScriptCore/jit/JITPowGenerator.h:27
> +#ifndef JITPowGenerator_h
> +#define JITPowGenerator_h

I think we're moving to "#pragma once"

> Source/JavaScriptCore/jit/JITPowGenerator.h:73
> +    SnippetOperand m_leftOperand;
> +    SnippetOperand m_rightOperand;
> +    JSValueRegs m_result;
> +    JSValueRegs m_left;
> +    JSValueRegs m_right;
> +    FPRReg m_leftFPR;
> +    FPRReg m_rightFPR;
> +    GPRReg m_scratchGPR;
> +    FPRReg m_scratchFPR;
> +    ArithProfile* m_arithProfile;
> +    bool m_didEmitFastPath { false };
> +    CCallHelpers::JumpList m_endJumpList;
> +    CCallHelpers::JumpList m_slowPathJumpList;

You should make this an IC, so follow Add's template (or you can also look at my Mul patch which extends what it means to be an IC a bit.)

> Source/JavaScriptCore/jit/JITPowGenerator.h:82
> +#define JITPowGenerator_h

oops

[Caio Lima]

Comment on attachment 284465 [details]
WIP Patch

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

Thanks for the comments. I am updating it with Math IC right in the next version.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4999
>> +    if (node->binaryUseKind() == UntypedUse) {
> 
> You should wait until my Mul IC patch lands to rebase this, and then this can easily follow the new templates I'm defining for being a Math IC.
> Otherwise, LGTM.

Nice. I am going to do it.

>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:174
>> +                } else if (yOperandValue == 2 && m_node->isBinaryUseKind(UntypedUse)) {
> 
> Why is this necessary?

For now yes. When the ArithMul is emitted, I got some problems in further steps. I am quite sure it is related with Clobberize rules.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2124
>> +            emitBinarySnippet<JITPowGenerator>(operationValuePow);
> 
> Ditto about waiting for the Mul IC to land, and then this can become an IC.
> Also, in WebKit, we don't use "{}" around single line body if/else/for/while statements

Nice catch. Sorry for that, because I am very use to use "{}" always and it is not the first time I do that.

>> Source/JavaScriptCore/jit/JITPowGenerator.cpp:36
>> +void JITPowGenerator::generateFastPath(CCallHelpers& jit)
> 
> I'll read this later. I'll trust that it's probably correct and derived from other code that emits pow code.

85% of it yes. The major difference is that I created a fast path for "in32t ** uint32".

[Caio Lima]

Created attachment 284757 [details]
Patch

[Caio Lima]

Perf analysis is coming soon.

[Caio Lima]

Created attachment 284760 [details]
Patch

[Keith Miller]

Looks like ews is sad.

[Caio Lima]

(In reply to comment #7)
> Looks like ews is sad.

I think I need rebase the code.

[Caio Lima]

Comment on attachment 284760 [details]
Patch

It is not working properly.

[Caio Lima]

Created attachment 284857 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1602:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1602:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 21 files


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

[Caio Lima]

Created attachment 284870 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1602:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1602:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 22 files


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

[Caio Lima]

Created attachment 284973 [details]
Benchmark run 01

[Caio Lima]

Created attachment 284974 [details]
Benchmark run 02

[Caio Lima]

Created attachment 284975 [details]
Benchmark run 03

[Caio Lima]

I ran benchmark comparing the Patch and the baseline. I noticed that it tends to regress the JSC ~1.02x. One of my guess is because of the overhead added by slow path compilation in all layers. We can consider don't emmit fast path when the Y operand is 0.5 or -0.5 and check benchmarks agin.

However, as '**' is an ES7 feature, I suppose it tends to be more used and IC and fast path tends to give more improvements than real regressions.

About memory comparison, I tried to run Unity 3D and JetStream benchmarks, but the op_pow wasn't emitted. Do you have in mind any other benchmark that we can use?

[Saam Barati]

Comment on attachment 284870 [details]
Patch

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

patch mostly LGTM, just a few suggestions

> Source/JavaScriptCore/jit/JITArithmetic.cpp:1000
> +//    JITSlowPathCall slowPathCall(this, currentInstruction, slow_path_pow);
> +//    slowPathCall.call();

Remove

> Source/JavaScriptCore/jit/JITOperations.cpp:2481
> +    auto nonOptimizeVariant = operationValuePowNoOptimize;
> +    powIC->generateOutOfLine(*vm, exec->codeBlock(), nonOptimizeVariant);

If you look at the code for ToT, you'll see that this variant tries to call observeLHSandRHS if the powIC has an ArithProfile.
That way, when we re-generate, we may emit better code by re-generating the inline path.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:53
> +    if (m_arithProfile) {
> +        lhs = m_arithProfile->lhsObservedType();
> +        rhs = m_arithProfile->rhsObservedType();
> +        if (lhs.isEmpty() || rhs.isEmpty()) {
> +            // FIXME: ICs should be able to repatch without emitting an inline path:
> +            // https://bugs.webkit.org/show_bug.cgi?id=160110
> +            lhs = ObservedType().withInt32();
> +            rhs = ObservedType().withInt32();
> +        }
> +    }

Please rebaseline, this is already fixed in ToT

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:81
> +        jit.boxInt32(m_scratchGPR, m_result);

Style: I'd either make all uses of m_scratchGPR here resultGpr, or remove the local resultGpr.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:124
> +bool JITPowGenerator::generateFastPath(CCallHelpers& jit, CCallHelpers::JumpList& endJumpList, CCallHelpers::JumpList& slowPathJumpList, bool shouldEmitProfiling)

Please add tests that test different variations of branches in this code.
Also, please add tests for exception handling of lhs/rhs valueOf in optimizing tiers.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:242
> +    if (m_arithProfile && shouldEmitProfiling)
> +        m_arithProfile->emitSetDouble(jit);

Do we use this information anywhere? If not, there is no need to emit this code.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:271
> +    if (m_arithProfile && shouldEmitProfiling)
> +        m_arithProfile->emitSetDouble(jit);

ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1340
>  _llint_op_pow:
>      traceExecution()
>      callOpcodeSlowPath(_slow_path_pow)
> -    dispatch(4)
> +    dispatch(5)

Please make slow_path_pow do type profiling for lhs/rhs.

[Caio Lima]

Created attachment 285246 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 25 files


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

[Saam Barati]

Comment on attachment 285246 [details]
Patch

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

I'm interested in seeing the latest results. I'm not sure off the top of my head why you're seeing such a huge regression.

> Source/JavaScriptCore/ChangeLog:10
> +        if exponentiation operator "**". Also, this patch includes Pow inline

s/if/of.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1571
> +        if (needsScratchFPRReg)
> +            patchpoint->numFPScratchRegisters = 3;
> +        else
> +            patchpoint->numFPScratchRegisters = 2;

Lets just make these numbers inputs to the function,
and in the patchpoint, you can do
FPRReg scratch = numFPScratchRegisters > 2 ? params.fpScratch(2) : InvalidFPRReg.
We can also do that with all scratches. I don't think all MathICs actually need two scratch FPRs
or one scratch GPR.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:67
> +        jit.push(xOperandGpr);
> +        jit.push(yOperandGpr);

Why are you doing this? This should definitely be written without pushing/popping from the stack.
Can it be written without clobbering the inputs? It seems like it'd be possible using a scratch.

[Caio Lima]

Created attachment 285304 [details]
New Benchmark

Now it is ok. Other benchmarks were dirty. I ran them with --outer 10

[Caio Lima]

Comment on attachment 285246 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1571
>> +            patchpoint->numFPScratchRegisters = 2;
> 
> Lets just make these numbers inputs to the function,
> and in the patchpoint, you can do
> FPRReg scratch = numFPScratchRegisters > 2 ? params.fpScratch(2) : InvalidFPRReg.
> We can also do that with all scratches. I don't think all MathICs actually need two scratch FPRs
> or one scratch GPR.

Nice! It also solves the problem abut push/pop variables in JITPowGenerator.

>> Source/JavaScriptCore/jit/JITPowGenerator.cpp:67
>> +        jit.push(yOperandGpr);
> 
> Why are you doing this? This should definitely be written without pushing/popping from the stack.
> Can it be written without clobbering the inputs? It seems like it'd be possible using a scratch.

The problem is the overflow. When it overflows, the xOperand and yOperand are clobbered and slowPath gets dirty operands.

I need 3 registers here. The current scratchGPR is being used as resultGPR.  One alternative is use 2 more scratchGPRs or use m_result.payloadGPR (this one I am 90% sure that m_result.payloadGPR == m_right.payloadGPeR, because I faced a bug using pop after jit.boxInt32).
Another solution is enable generators to use more scratchGPRs, as mentioned before.

One other possible solution is don't emmit code as int32 ** int32 cases and use just use double version with int32 -> double cast when leftOperand is an int32. I am considering this case, because of overflow check in each iteration can be costly and worst case is when overflow happens and we need calculate everything from scratch in slowPath.  Checking this article (http://nicolas.limare.net/pro/notes/2014/12/12_arit_speed/) comparing int32 * int32 against float64 * float64 in some architectures has a factor of ~1.6x and maybe use mul32 can't be worth because there is a highly probability of overflow.

[Caio Lima]

Created attachment 285406 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 25 files


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

[Caio Lima]

(In reply to comment #21)
> Comment on attachment 285246 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285246&action=review
> 
> I'm interested in seeing the latest results. I'm not sure off the top of my
> head why you're seeing such a huge regression.
> 
> > Source/JavaScriptCore/ChangeLog:10
> > +        if exponentiation operator "**". Also, this patch includes Pow inline
> 
> s/if/of.
> 
> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:1571
> > +        if (needsScratchFPRReg)
> > +            patchpoint->numFPScratchRegisters = 3;
> > +        else
> > +            patchpoint->numFPScratchRegisters = 2;
> 
> Lets just make these numbers inputs to the function,
> and in the patchpoint, you can do
> FPRReg scratch = numFPScratchRegisters > 2 ? params.fpScratch(2) :
> InvalidFPRReg.
> We can also do that with all scratches. I don't think all MathICs actually
> need two scratch FPRs
> or one scratch GPR.
> 
> > Source/JavaScriptCore/jit/JITPowGenerator.cpp:67
> > +        jit.push(xOperandGpr);
> > +        jit.push(yOperandGpr);
> 
> Why are you doing this? This should definitely be written without
> pushing/popping from the stack.
> Can it be written without clobbering the inputs? It seems like it'd be
> possible using a scratch.
 In an IRC conversation with Saam, I am going to remove the push/pop usage in this bug (https://bugs.webkit.org/show_bug.cgi?id=160588).

[Saam Barati]

Comment on attachment 285406 [details]
Patch

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

LGTM, but I think I found a bug. so r- for now just to verify/fix bug.

> Source/JavaScriptCore/ChangeLog:12
> +        cache when generating JITed code in all tiers. Cases considered in Pow
> +        IC are "Int32 ** UInt32" and "Double ** UInt32".

Can you post some stats for the average size of these ICs?

> Source/JavaScriptCore/ChangeLog:14
> +        The algorithm used in fast paths has log(lhs) complexity and its idea

you mean log(rhs) here.

> Source/JavaScriptCore/jit/JITOperations.cpp:2459
> +    double a = op1.toNumber(exec);
> +    double b = op2.toNumber(exec);

Please rebase to ToT. There should be an exception check between these two toNumber operations, and I've added tests for it. I believe this code will fail that test.

> Source/JavaScriptCore/jit/JITOperations.cpp:2472
> +    double a = op1.toNumber(exec);
> +    double b = op2.toNumber(exec);

ditto

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:57
> +    if (lhs.isOnlyInt32() && rhs.isOnlyInt32()) {
> +        ASSERT(!m_leftOperand.isPositiveConstInt32() || !m_rightOperand.isPositiveConstInt32());
> +        if (!m_leftOperand.isPositiveConstInt32())
> +            state.slowPathJumps.append(jit.branchIfNotInt32(m_left));
> +        if (!m_rightOperand.isPositiveConstInt32())
> +            state.slowPathJumps.append(jit.branchIfNotInt32(m_right));

Hmm, this code seems slightly wrong. You don't seem to handle the loading of a lhs/rhs constant manually.
You should add tests for this, I suspect they'll fail. It's worth looking at other tests that test snippets
and adding one for pow. Look at JSTests/stress/op_add.js, op_mul.js, etc.

Please add constant tests for other paths as well.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:66
> +        // FIXME: Trun the mechanism of GPScratchRegister and FPScratchRegisters more flexible.

typo: trun => turn

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:173
> +    // FIXME: Trun the mechanism of GPScratchRegister and FPScratchRegisters more flexible.

ditto typo

> JSTests/stress/pow-ic.js:3
> +function valuesAreClose(a, b) {
> +    return Math.abs(a / b) - 1 < 1e-10;
> +}

Why is this needed?

> JSTests/stress/pow-untyped-fastpath.js:1
> +function valuesAreClose(a, b) {

Can you add a test that throws exceptions in all tiers as well?

[Caio Lima]

(In reply to comment #27)
> Comment on attachment 285406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285406&action=review
> 
> LGTM, but I think I found a bug. so r- for now just to verify/fix bug.
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        cache when generating JITed code in all tiers. Cases considered in Pow
> > +        IC are "Int32 ** UInt32" and "Double ** UInt32".
> 
> Can you post some stats for the average size of these ICs?

I tried use the Unity3d to stat this info, however ArithPow isn't inlined in this benchmark. I can do that get it from some piece of test codes. Is that ok to you?

> > Source/JavaScriptCore/ChangeLog:14
> > +        The algorithm used in fast paths has log(lhs) complexity and its idea
> 
> you mean log(rhs) here.

Right.

> > Source/JavaScriptCore/jit/JITOperations.cpp:2459
> > +    double a = op1.toNumber(exec);
> > +    double b = op2.toNumber(exec);
> 
> Please rebase to ToT. There should be an exception check between these two
> toNumber operations, and I've added tests for it. I believe this code will
> fail that test.
> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:2472
> > +    double a = op1.toNumber(exec);
> > +    double b = op2.toNumber(exec);
> 
> ditto
> 
> > Source/JavaScriptCore/jit/JITPowGenerator.cpp:57
> > +    if (lhs.isOnlyInt32() && rhs.isOnlyInt32()) {
> > +        ASSERT(!m_leftOperand.isPositiveConstInt32() || !m_rightOperand.isPositiveConstInt32());
> > +        if (!m_leftOperand.isPositiveConstInt32())
> > +            state.slowPathJumps.append(jit.branchIfNotInt32(m_left));
> > +        if (!m_rightOperand.isPositiveConstInt32())
> > +            state.slowPathJumps.append(jit.branchIfNotInt32(m_right));
> 
> Hmm, this code seems slightly wrong. You don't seem to handle the loading of
> a lhs/rhs constant manually.
> You should add tests for this, I suspect they'll fail. It's worth looking at
> other tests that test snippets
> and adding one for pow. Look at JSTests/stress/op_add.js, op_mul.js, etc.
> 
> Please add constant tests for other paths as well.

You are right. The correct way actually is to never consider left or right operand valid constants as JITSubGenerator is doing, since I need these values in some registers to perform fast paths.
 
> > Source/JavaScriptCore/jit/JITPowGenerator.cpp:66
> > +        // FIXME: Trun the mechanism of GPScratchRegister and FPScratchRegisters more flexible.
> 
> typo: trun => turn
> 
> > Source/JavaScriptCore/jit/JITPowGenerator.cpp:173
> > +    // FIXME: Trun the mechanism of GPScratchRegister and FPScratchRegisters more flexible.
> 
> ditto typo
> 
> > JSTests/stress/pow-ic.js:3
> > +function valuesAreClose(a, b) {
> > +    return Math.abs(a / b) - 1 < 1e-10;
> > +}
> 
> Why is this needed?

Because of Float/double precision on comparison. It is safer use this because 1.0 + 1.0 == 2.0 can be false sometimes because of precision lost in double operations. Is this make sense?

> > JSTests/stress/pow-untyped-fastpath.js:1
> > +function valuesAreClose(a, b) {
> 
> Can you add a test that throws exceptions in all tiers as well?

Sure.

[Caio Lima]

Created attachment 286007 [details]
PatchToBot

[Caio Lima]

Created attachment 286008 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1611:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 25 files


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

[Caio Lima]

Created attachment 300584 [details]
Patch

[Caio Lima]

Let's see what EWS thinks about this Patch.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 33 files


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

[Caio Lima]

Created attachment 300631 [details]
Patch

[Caio Lima]

Created attachment 300636 [details]
Patch Updated

Rebased with upstream/master. Hope it builds now.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 33 files


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

[Caio Lima]

Created attachment 300640 [details]
Patch Updated

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1723:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 33 files


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

[Caio Lima]

Created attachment 302013 [details]
Patch

[Caio Lima]

Rebased with master

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1724:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1724:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 33 files


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

[Saam Barati]

Comment on attachment 302013 [details]
Patch

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

Seems like a good start. There look like there are a few bugs though. I also have a few suggestions. Comments below.

> Source/JavaScriptCore/ChangeLog:10
> +        the indorduction of exponentiation operator "**".

typo: "indorduction" => "introduction"

> Source/JavaScriptCore/ChangeLog:19
> +        The algorithm used in fast paths has log(rhs) complexity and its idea
> +        is calculate the result dividing the exponent per 2 in each interation.
> +        We also consider cases where exponent is 0.5 and we can use sqrt to
> +        calculate the result.

I think most people reading this are probably aware of the algorithm, however, it might be worth adding a link to something that explains it.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:901
> +        if (node->isBinaryUseKind(UntypedUse)) {
> +            clobberWorld(node->origin.semantic, clobberLimit);
> +            forNode(node).setType(m_graph, SpecBytecodeNumber);
> +            break;
> +        }

You should check the semantics of "a**b", if they're
toNumber(a) ** toNumber(b) [or something similar],
then the above constant folding rule inside AI is wrong, since it will skip the toNumber(a). This is observable w/ objects and such.
Please add a test for this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5384
> +    if (node->binaryUseKind() == UntypedUse) {

The above code is broken if "node->child1().useKind() == UntypedUse && node->child2().isDoubleConstant()"
You probably want this if as the first thing in the program.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:46
> +#if USE(JSVALUE32_64)
> +    UNUSED_PARAM(jit);
> +    UNUSED_PARAM(state);
> +    UNUSED_PARAM(arithProfile);
> +    return JITMathICInlineResult::DontGenerate;

This seems basically useless. We're going to end up being slower than just going to a C call immediately. Why not just never even emit an IC for 32 bit platforms? I'd just conditionally compile out this entire class for 32 bit platforms.

> Source/JavaScriptCore/jit/JITPowGenerator.h:2
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

2017

[Caio Lima]

Created attachment 303489 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1724:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1724:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Caio Lima]

Created attachment 304771 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1737:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1737:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Caio Lima]

Created attachment 305275 [details]
Patch new proposal

This Patch I rebased the code and also fixed some NITs.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Caio Lima]

Ping review

[Caio Lima]

Created attachment 306289 [details]
Patch fixed

Rebasing Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Caio Lima]

Created attachment 306346 [details]
Fixed Patch

Fixing last Patch compilation error.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Caio Lima]

Created attachment 307175 [details]
Proposed Patch

Fixing some NITs and on bug found into IC implementation.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1734:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 35 files


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

[Saam Barati]

Comment on attachment 307175 [details]
Proposed Patch

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

Nice, patch mostly LGTM. Just some comments, mostly style related.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:367
>          case ArithPow: {
> +            Edge& leftChild = node->child1();
> +            Edge& rightChild = node->child2();

You need to mark this node as MustGenerate inside DFGNodeType.h, and then you can clear this flag if we don't have UntypedUse.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3702
> +    const bool needsScratchGPRReg = Generator::scratchGPRCount > 0;
> +    const bool needsScratchFPRReg = Generator::scratchFPRCount > 0;

Why is this a number if we only ever allocate a single scratch? Maybe assert it's only ever 1 if we get here at runtime?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5356
> +    std::optional<FPRTemporary> fprScratch[scratchFPRsCount];

Why are these optionals?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5447
> +        silentSpillAllRegisters(resultRegs);
> +
> +        if (leftOperand.isConst()) {
> +            leftRegs = resultRegs;
> +            m_jit.moveValue(leftChild->asJSValue(), leftRegs);
> +        }
> +        if (rightOperand.isConst()) {
> +            rightRegs = resultRegs;
> +            m_jit.moveValue(rightChild->asJSValue(), rightRegs);
> +        }
> +
> +        callOperation(operationValuePow, resultRegs, leftRegs, rightRegs);
> +
> +        silentFillAllRegisters(resultRegs);

Since you always make a call, there is no need for silentSpill/silentFill. Instead, you should just unconditionally flushRegisters.

> Source/JavaScriptCore/jit/JITAddGenerator.h:65
> +        if (scratchFPRCount > 0)

Style nit: no need for > 0, just "if (scratchFPRCount)"

> Source/JavaScriptCore/jit/JITArithmetic.cpp:697
> +    scratchGPRegs[0] = regT3;
> +    scratchGPRegs[1] = regT4;
> +    scratchGPRegs[2] = regT5;
> +    
> +    scratchFPRegs[0] = fpRegT2;

Please static_assert the sizes since this is hand coded.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:837
> +    if (Generator::scratchFPRCount > 0)
> +        scratchFPRegs.append(scratchFPR);

Same comment, seems you should assert it's 1 if it's greater than zero.

> Source/JavaScriptCore/jit/JITOperations.cpp:2634
> +    if (UNLIKELY(scope.exception()))
> +        return JSValue::encode(JSValue());

Use RETURN_IF_EXCEPTION

> Source/JavaScriptCore/jit/JITOperations.cpp:2637
> +    if (UNLIKELY(scope.exception()))
> +        return JSValue::encode(JSValue());

Ditto

> Source/JavaScriptCore/jit/JITOperations.cpp:2653
> +    if (UNLIKELY(scope.exception()))
> +        return JSValue::encode(JSValue());

Ditto

> Source/JavaScriptCore/jit/JITOperations.cpp:2656
> +    if (UNLIKELY(scope.exception()))
> +        return JSValue::encode(JSValue());

Ditto

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:45
> +    if (DISABLE_INLINE)
> +        return JITMathICInlineResult::DontGenerate;

Why have this?

> Source/JavaScriptCore/jit/JITSubGenerator.h:63
> +        if (scratchFPRCount > 0)

Style nit: this should just be "if (scratchFPRCount)", and then if true, I'd runtime assert it's ==1.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:537
> -    double a = OP_C(2).jsValue().toNumber(exec);
> +    JSValue left = OP_C(2).jsValue();
> +    double a = left.toNumber(exec);
>      if (UNLIKELY(throwScope.exception()))
>          RETURN(JSValue());
> -    double b = OP_C(3).jsValue().toNumber(exec);
> +    JSValue right = OP_C(3).jsValue();
> +    double b = right.toNumber(exec);
>      if (UNLIKELY(throwScope.exception()))
>          RETURN(JSValue());
> +    ArithProfile* arithProfile = exec->codeBlock()->arithProfileForPC(pc);
> +    arithProfile->observeLHSAndRHS(left, right);
>      RETURN(jsNumber(operationMathPow(a, b)));

Is this code only reachable via the LLInt? If so, please move it to LLIntSlowPaths instead of CommonSlowPaths.

[Caio Lima]

Comment on attachment 307175 [details]
Proposed Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3702
>> +    const bool needsScratchFPRReg = Generator::scratchFPRCount > 0;
> 
> Why is this a number if we only ever allocate a single scratch? Maybe assert it's only ever 1 if we get here at runtime?

It can be 0 and 1 depending the arch. However this number is used to keep "FTLLowerDFGToB3.cpp" compileMathIC() code generic as well.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5356
>> +    std::optional<FPRTemporary> fprScratch[scratchFPRsCount];
> 
> Why are these optionals?

It's from the old version. We don't need that anymore. Nice Catch.

>> Source/JavaScriptCore/jit/JITPowGenerator.cpp:45
>> +        return JITMathICInlineResult::DontGenerate;
> 
> Why have this?

Debug code. My bad.

>> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:537
>>      RETURN(jsNumber(operationMathPow(a, b)));
> 
> Is this code only reachable via the LLInt? If so, please move it to LLIntSlowPaths instead of CommonSlowPaths.

Yes. It is used in void JIT::emit_op_pow(Instruction* currentInstruction) in JITArithmetic.cpp.

[Caio Lima]

Created attachment 307692 [details]
proposed Patch

Covering Saam's comments.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Build Bot]

Comment on attachment 307692 [details]
proposed Patch

Attachment 307692 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3574320

New failing tests:
microbenchmarks/int-pow.js.ftl-eager
microbenchmarks/float-pow.js.ftl-eager
microbenchmarks/float-pow.js.no-cjit-validate-phases
microbenchmarks/int-pow.js.default
microbenchmarks/float-pow.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/int-pow.js.no-ftl
microbenchmarks/int-pow.js.dfg-eager
microbenchmarks/int-pow.js.ftl-eager-no-cjit
microbenchmarks/int-pow.js.ftl-no-cjit-validate-sampling-profiler
stress/pow-untyped-fastpath.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/float-pow.js.dfg-eager-no-cjit-validate
stress/pow-untyped-fastpath.js.default
stress/pow-untyped-fastpath.js.no-cjit-validate-phases
stress/pow-untyped-fastpath.js.ftl-eager-no-cjit-b3o1
stress/pow-untyped-fastpath.js.ftl-no-cjit-small-pool
stress/pow-untyped-fastpath.js.no-llint
microbenchmarks/int-pow.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/int-pow.js.no-cjit-collect-continuously
microbenchmarks/float-pow.js.dfg-eager
microbenchmarks/float-pow.js.no-ftl
microbenchmarks/int-pow.js.dfg-eager-no-cjit-validate
microbenchmarks/int-pow.js.ftl-no-cjit-no-inline-validate
microbenchmarks/float-pow.js.default
microbenchmarks/float-pow.js.ftl-eager-no-cjit
microbenchmarks/float-pow.js.ftl-eager-no-cjit-b3o1
stress/regress-159779-1.js.ftl-no-cjit
stress/pow-untyped-fastpath.js.dfg-eager
microbenchmarks/float-pow.js.no-cjit-collect-continuously
stress/pow-untyped-fastpath.js.no-ftl
microbenchmarks/float-pow.js.ftl-no-cjit-small-pool
stress/pow-untyped-fastpath.js.ftl-eager-no-cjit
stress/pow-untyped-fastpath.js.ftl-eager
stress/pow-untyped-fastpath.js.ftl-no-cjit-no-inline-validate
stress/regress-159779-1.js.no-ftl
stress/pow-untyped-fastpath.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/int-pow.js.no-cjit-validate-phases
microbenchmarks/int-pow.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/float-pow.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/float-pow.js.ftl-no-cjit-no-inline-validate
microbenchmarks/int-pow.js.ftl-no-cjit-small-pool
microbenchmarks/int-pow.js.no-llint
stress/regress-159779-1.js.ftl-eager-no-cjit
microbenchmarks/int-pow.js.ftl-eager-no-cjit-b3o1
microbenchmarks/float-pow.js.ftl-no-cjit-b3o1
stress/pow-untyped-fastpath.js.no-cjit-collect-continuously
microbenchmarks/float-pow.js.ftl-no-cjit-validate-sampling-profiler
stress/regress-159779-1.js.default
stress/pow-untyped-fastpath.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/int-pow.js.ftl-no-cjit-b3o1
stress/regress-159779-2.js.ftl-no-cjit
stress/pow-untyped-fastpath.js.ftl-no-cjit-b3o1
stress/pow-untyped-fastpath.js.dfg-eager-no-cjit-validate
microbenchmarks/float-pow.js.no-llint

[Caio Lima]

Created attachment 307745 [details]
proposed Patch

Fixing regressions.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Build Bot]

Comment on attachment 307745 [details]
proposed Patch

Attachment 307745 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3577287

New failing tests:
webrtc/datachannel/bufferedAmountLowThreshold.html

[Build Bot]

Created attachment 307767 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Caio Lima]

Created attachment 307772 [details]
proposed Patch

New Patch Rebased.

[Build Bot]

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


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:122:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/network/MIMEHeader.h:31:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/WebCoreAVFResourceLoader.mm:189:  Use std::min() or std::min<type>() instead of the MIN() macro.  [runtime/max_min_macros] [4]
ERROR: Source/WebCore/platform/graphics/GeometryUtilities.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WebCore/platform/SharedBufferChunkReader.h:31:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WebCore/platform/cf/SharedBufferCF.cpp:160:  Declaration has space between type name and * in const char *SharedBuffer  [whitespace/declaration] [3]
ERROR: Source/WebCore/platform/SharedBuffer.cpp:364:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 11 in 131 files


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

[Caio Lima]

Created attachment 307774 [details]
Proposed Patch

Last Version was the wrong diff.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5388:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Saam Barati]

Comment on attachment 307175 [details]
Proposed Patch

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

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3702
>>> +    const bool needsScratchFPRReg = Generator::scratchFPRCount > 0;
>> 
>> Why is this a number if we only ever allocate a single scratch? Maybe assert it's only ever 1 if we get here at runtime?
> 
> It can be 0 and 1 depending the arch. However this number is used to keep "FTLLowerDFGToB3.cpp" compileMathIC() code generic as well.

What I'm saying is, b/c it can be zero or one, if this boolean is true, assert that it's 1 and not some number larger than 1. (since this code wouldn't work for a number larger than 1)

[Caio Lima]

(In reply to Saam Barati from comment #70)
> Comment on attachment 307175 [details]
> Proposed Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307175&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3702
> >>> +    const bool needsScratchFPRReg = Generator::scratchFPRCount > 0;
> >> 
> >> Why is this a number if we only ever allocate a single scratch? Maybe assert it's only ever 1 if we get here at runtime?
> > 
> > It can be 0 and 1 depending the arch. However this number is used to keep "FTLLowerDFGToB3.cpp" compileMathIC() code generic as well.
> 
> What I'm saying is, b/c it can be zero or one, if this boolean is true,
> assert that it's 1 and not some number larger than 1. (since this code
> wouldn't work for a number larger than 1)

Gotcha! I understood that I should replace it to their bool version. Changing it now.

[Caio Lima]

Created attachment 307828 [details]
Proposed Patch

Possible Patch for landing.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5393:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1736:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Caio Lima]

Created attachment 308822 [details]
Proposed Patch

Rebased with master and ping for review

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5393:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Saam Barati]

Comment on attachment 308822 [details]
Proposed Patch

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

some comments below. Mostly LGTM

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3754
> +    JSValueRegs resultRegs = JSValueRegs(resultPayload.gpr(), resultTag.gpr());

I don't think this actually matters, but the correct constructor takes tag first, and payload second. Please change ordering so your naming matches the constructor.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5409
> +    if (node->child1().useKind() == UntypedUse && node->child2().useKind() == UntypedUse) {

Nit: I'd use "isBinaryUseKind(UntypedUse)" just to be consistent with the code elsewhere.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5413
> +        if (isKnownNotNumber(leftChild.node()) && isKnownNotNumber(rightChild.node())) {

This should be ||, not and. You're gonna go to the slow path if either is not a number, not if both are not a number, right?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5473
> +        SnippetOperand leftOperand(m_state.forNode(leftChild).resultType());
> +        SnippetOperand rightOperand(m_state.forNode(rightChild).resultType());
> +
> +        if (leftChild->isInt32Constant())
> +            leftOperand.setConstInt32(leftChild->asInt32());
> +        else if (rightChild->isInt32Constant())
> +            rightOperand.setConstInt32(rightChild->asInt32());
> +
> +        ASSERT(!leftOperand.isConst() || !rightOperand.isConst());
> +
> +        if (!leftOperand.isConst()) {
> +            left.emplace(this, leftChild);
> +            leftRegs = left->jsValueRegs();
> +        }
> +        if (!rightOperand.isConst()) {
> +            right.emplace(this, rightChild);
> +            rightRegs = right->jsValueRegs();
> +        }
> +
> +        if (leftOperand.isConst()) {
> +            leftRegs = resultRegs;
> +            m_jit.moveValue(leftChild->asJSValue(), leftRegs);
> +        }
> +        if (rightOperand.isConst()) {
> +            rightRegs = resultRegs;
> +            m_jit.moveValue(rightChild->asJSValue(), rightRegs);
> +        }

Why even bother? I don't think this buys you anything. I'd just operate as if nothing is a constant for 32-bit platforms.

> Source/JavaScriptCore/jit/JITArithmetic.cpp:842
> +    if (Generator::scratchFPRCount > 0) {
> +        ASSERT(scratchFPR != InvalidFPRReg);
> +        scratchFPRegs.append(scratchFPR);
> +    }

Please assert it's one. Otherwise, this code is clearly wrong.

> Source/JavaScriptCore/jit/JITMulGenerator.h:66
> +        if (scratchFPRCount)
> +            m_scratchFPR = scratchFPRegs[0];

Nit: I'd add an "else m_scratchFPR = InvalidFPRReg" (and for any other constructors that do the same thing).

> Source/JavaScriptCore/jit/JITOperations.cpp:2732
> +#endif

You can just have a single `#if USE(JSVALUE64)` range for many of these since you just repeat it around each individual function.

[Caio Lima]

Created attachment 309662 [details]
proposed Patch

Rebased Patch with master.

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5383:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Build Bot]

Comment on attachment 309662 [details]
proposed Patch

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

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html

[Build Bot]

Created attachment 309683 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Caio Lima]

Created attachment 309711 [details]
Patch for landing

Comments fixed. Let's see what EWS thinks about it.

[Build Bot]

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


ERROR: Source/JavaScriptCore/jit/JITArithmetic.cpp:845:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5383:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 36 files


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

[Build Bot]

Comment on attachment 309711 [details]
Patch for landing

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 309718 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Caio Lima]

Created attachment 309720 [details]
Patch for landing

Fixing regressions

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5383:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Caio Lima]

Created attachment 311897 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5378:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 3 in 36 files


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

[Saam Barati]

Comment on attachment 311897 [details]
Patch

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

still mostly LGTM, but I have some general comments. I think this is very close to being ready to land.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:933
> +        if (node->isBinaryUseKind(UntypedUse)) {
> +            clobberWorld(node->origin.semantic, clobberLimit);
> +            forNode(node).setType(m_graph, SpecBytecodeNumber);
> +            break;
> +        }

this doesn't look exactly correct. I think you want some of the constant folding below to kick in. But perhaps not all of it. e.g, if both X and Y are constants, and constant numbers, then we want to constant fold. It seems unlikely we'll prove them as constants and have Untyped use here, but it is possible. You need to be careful though as to not let the first rule kick un for UntypedUse since it could remove effects from the program.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5398
> +        if (isKnownNotNumber(leftChild.node()) || isKnownNotNumber(rightChild.node())) {

Do you have a test for this?

> Source/JavaScriptCore/jit/JITAddGenerator.h:46
> +    static const size_t scratchGPRCount = 1;
> +#if USE(JSVALUE64)
> +    static const size_t scratchFPRCount = 0;
> +#else
> +    static const size_t scratchFPRCount = 1;
> +#endif

Nit: I think all of these can be constexpr

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

Why? This is unused.

> Source/JavaScriptCore/jit/JITOperations.cpp:2638
> +    RETURN_IF_EXCEPTION(scope, JSValue::encode(JSValue()));

style nit: We usually do this instead:
RETURN_IF_EXCEPTION(scope, { });
(Same comment for below code)

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

no need for Apple here, since this is your code.

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:51
> +    if (lhs.isOnlyNonNumber() && rhs.isOnlyNonNumber())

Why not || here?

> Source/JavaScriptCore/jit/JITPowGenerator.cpp:70
> +        // FIXME: Turn the mechanism of GPScratchRegister and FPScratchRegisters more flexible.
> +        // https://bugs.webkit.org/show_bug.cgi?id=160588

Is this still needed?

> Source/JavaScriptCore/jit/JITPowGenerator.h:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

no need for Apple here, since this is your code.

[Caio Lima]

Created attachment 311970 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1754:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Caio Lima]

Created attachment 312148 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1761:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1761:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Caio Lima]

Created attachment 313803 [details]
Patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1752:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 37 files


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

[Maciej Stachowiak]

Comment on attachment 313803 [details]
Patch

I don't have enough expertise in this area to properly review, but in any case this patch no longer applies cleanly.

[Caio Lima]

This bug isn't valid anymore. We now support ValuePow(UntypedUse) because of BigInt.