Bug 160012 - [DFG] Support ArithPow(Untyped, Untyped)
Summary: [DFG] Support ArithPow(Untyped, Untyped)
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Lima
URL:
Keywords:
Depends on: 160588 160802
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-21 00:01 PDT by Yusuke Suzuki
Modified: 2017-06-25 08:18 PDT (History)
7 users (show)

See Also:


Attachments
WIP Patch (19.37 KB, patch)
2016-07-25 00:05 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (37.60 KB, patch)
2016-07-28 00:27 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (37.60 KB, patch)
2016-07-28 01:07 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (43.21 KB, patch)
2016-07-29 03:04 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (43.74 KB, patch)
2016-07-29 10:27 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark run 01 (78.00 KB, text/plain)
2016-07-31 17:01 PDT, Caio Lima
no flags Details
Benchmark run 02 (79.91 KB, text/plain)
2016-07-31 17:02 PDT, Caio Lima
no flags Details
Benchmark run 03 (80.05 KB, text/plain)
2016-07-31 17:02 PDT, Caio Lima
no flags Details
Patch (52.09 KB, patch)
2016-08-03 10:47 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
New Benchmark (78.68 KB, text/plain)
2016-08-03 23:36 PDT, Caio Lima
no flags Details
Patch (52.08 KB, patch)
2016-08-05 00:02 PDT, Caio Lima
sbarati: review-
Details | Formatted Diff | Diff
PatchToBot (52.20 KB, patch)
2016-08-13 15:07 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (53.83 KB, patch)
2016-08-13 15:20 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (80.87 KB, patch)
2017-02-03 17:57 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (82.31 KB, patch)
2017-02-04 10:08 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch Updated (79.63 KB, patch)
2017-02-04 12:46 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch Updated (79.36 KB, patch)
2017-02-04 15:14 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (81.52 KB, patch)
2017-02-17 16:53 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (85.35 KB, patch)
2017-03-05 17:37 PST, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (85.31 KB, patch)
2017-03-17 05:28 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch new proposal (82.94 KB, patch)
2017-03-24 04:55 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch fixed (82.84 KB, patch)
2017-04-05 09:28 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Fixed Patch (82.83 KB, patch)
2017-04-05 17:44 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (82.95 KB, patch)
2017-04-14 18:01 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
proposed Patch (83.67 KB, patch)
2017-04-20 22:15 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
proposed Patch (83.67 KB, patch)
2017-04-21 10:10 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (903.84 KB, application/zip)
2017-04-21 12:01 PDT, Build Bot
no flags Details
proposed Patch (407.47 KB, patch)
2017-04-21 12:21 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (83.66 KB, patch)
2017-04-21 12:30 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (83.82 KB, patch)
2017-04-21 16:21 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (83.91 KB, patch)
2017-05-02 07:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
proposed Patch (83.84 KB, patch)
2017-05-10 17:40 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.76 MB, application/zip)
2017-05-10 20:17 PDT, Build Bot
no flags Details
Patch for landing (83.06 KB, patch)
2017-05-11 08:39 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (395.51 KB, application/zip)
2017-05-11 09:47 PDT, Build Bot
no flags Details
Patch for landing (83.00 KB, patch)
2017-05-11 10:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (85.34 KB, patch)
2017-06-02 19:09 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (86.13 KB, patch)
2017-06-04 13:33 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (86.17 KB, patch)
2017-06-06 17:54 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Patch (86.02 KB, patch)
2017-06-25 08:17 PDT, Caio Lima
ticaiolima: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-07-21 00:01:04 PDT
Now, ** operator is supported. So it is consistent that we support ArithPow(Untyped, Untyped), as similar as ArithSub(Untyped, Untyped), ArithMul(Untyped, Untyped) etc.
Comment 1 Caio Lima 2016-07-25 00:05:32 PDT
Created attachment 284465 [details]
WIP Patch

RFC in this implementation.
Comment 2 Saam Barati 2016-07-25 10:53:18 PDT
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
Comment 3 Caio Lima 2016-07-26 09:55:42 PDT
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".
Comment 4 Caio Lima 2016-07-28 00:27:23 PDT
Created attachment 284757 [details]
Patch
Comment 5 Caio Lima 2016-07-28 00:34:50 PDT
Perf analysis is coming soon.
Comment 6 Caio Lima 2016-07-28 01:07:49 PDT
Created attachment 284760 [details]
Patch
Comment 7 Keith Miller 2016-07-28 09:17:55 PDT
Looks like ews is sad.
Comment 8 Caio Lima 2016-07-28 09:35:58 PDT
(In reply to comment #7)
> Looks like ews is sad.

I think I need rebase the code.
Comment 9 Caio Lima 2016-07-28 10:30:03 PDT
Comment on attachment 284760 [details]
Patch

It is not working properly.
Comment 10 Caio Lima 2016-07-29 03:04:30 PDT
Created attachment 284857 [details]
Patch
Comment 11 WebKit Commit Bot 2016-07-29 03:06:49 PDT
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.
Comment 12 Caio Lima 2016-07-29 10:27:12 PDT
Created attachment 284870 [details]
Patch
Comment 13 WebKit Commit Bot 2016-07-29 10:29:56 PDT
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.
Comment 14 Caio Lima 2016-07-31 17:01:37 PDT
Created attachment 284973 [details]
Benchmark run 01
Comment 15 Caio Lima 2016-07-31 17:02:06 PDT
Created attachment 284974 [details]
Benchmark run 02
Comment 16 Caio Lima 2016-07-31 17:02:31 PDT
Created attachment 284975 [details]
Benchmark run 03
Comment 17 Caio Lima 2016-07-31 17:18:40 PDT
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?
Comment 18 Saam Barati 2016-08-01 11:50:00 PDT
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.
Comment 19 Caio Lima 2016-08-03 10:47:33 PDT
Created attachment 285246 [details]
Patch
Comment 20 WebKit Commit Bot 2016-08-03 10:50:43 PDT
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.
Comment 21 Saam Barati 2016-08-03 14:39:53 PDT
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.
Comment 22 Caio Lima 2016-08-03 23:36:38 PDT
Created attachment 285304 [details]
New Benchmark

Now it is ok. Other benchmarks were dirty. I ran them with --outer 10
Comment 23 Caio Lima 2016-08-04 00:09:46 PDT
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.
Comment 24 Caio Lima 2016-08-05 00:02:22 PDT
Created attachment 285406 [details]
Patch
Comment 25 WebKit Commit Bot 2016-08-05 00:05:54 PDT
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.
Comment 26 Caio Lima 2016-08-05 00:07:57 PDT
(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).
Comment 27 Saam Barati 2016-08-09 16:34:14 PDT
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?
Comment 28 Caio Lima 2016-08-13 13:30:54 PDT
(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.
Comment 29 Caio Lima 2016-08-13 15:07:39 PDT
Created attachment 286007 [details]
PatchToBot
Comment 30 Caio Lima 2016-08-13 15:20:34 PDT
Created attachment 286008 [details]
Patch
Comment 31 WebKit Commit Bot 2016-08-13 15:22:22 PDT
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.
Comment 32 Caio Lima 2017-02-03 17:57:38 PST
Created attachment 300584 [details]
Patch
Comment 33 Caio Lima 2017-02-03 17:58:46 PST
Let's see what EWS thinks about this Patch.
Comment 34 WebKit Commit Bot 2017-02-03 18:00:21 PST
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.
Comment 35 Caio Lima 2017-02-04 10:08:52 PST
Created attachment 300631 [details]
Patch
Comment 36 Caio Lima 2017-02-04 12:46:39 PST
Created attachment 300636 [details]
Patch Updated

Rebased with upstream/master. Hope it builds now.
Comment 37 WebKit Commit Bot 2017-02-04 12:49:13 PST
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.
Comment 38 Caio Lima 2017-02-04 15:14:30 PST
Created attachment 300640 [details]
Patch Updated
Comment 39 WebKit Commit Bot 2017-02-04 15:16:43 PST
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.
Comment 40 Caio Lima 2017-02-17 16:53:31 PST
Created attachment 302013 [details]
Patch
Comment 41 Caio Lima 2017-02-17 16:54:22 PST
Rebased with master
Comment 42 WebKit Commit Bot 2017-02-17 16:55:59 PST
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.
Comment 43 Saam Barati 2017-02-21 15:10:11 PST
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
Comment 44 Caio Lima 2017-03-05 17:37:09 PST
Created attachment 303489 [details]
Patch
Comment 45 WebKit Commit Bot 2017-03-05 17:39:24 PST
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.
Comment 46 Caio Lima 2017-03-17 05:28:33 PDT
Created attachment 304771 [details]
Patch
Comment 47 WebKit Commit Bot 2017-03-17 05:31:05 PDT
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.
Comment 48 Caio Lima 2017-03-24 04:55:14 PDT
Created attachment 305275 [details]
Patch new proposal

This Patch I rebased the code and also fixed some NITs.
Comment 49 Build Bot 2017-03-24 04:58:34 PDT
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.
Comment 50 Caio Lima 2017-03-27 06:53:37 PDT
Ping review
Comment 51 Caio Lima 2017-04-05 09:28:19 PDT
Created attachment 306289 [details]
Patch fixed

Rebasing Patch
Comment 52 Build Bot 2017-04-05 09:29:56 PDT
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.
Comment 53 Caio Lima 2017-04-05 17:44:30 PDT
Created attachment 306346 [details]
Fixed Patch

Fixing last Patch compilation error.
Comment 54 Build Bot 2017-04-05 17:46:59 PDT
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.
Comment 55 Caio Lima 2017-04-14 18:01:46 PDT
Created attachment 307175 [details]
Proposed Patch

Fixing some NITs and on bug found into IC implementation.
Comment 56 Build Bot 2017-04-14 18:03:50 PDT
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.
Comment 57 Saam Barati 2017-04-17 20:05:18 PDT
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.
Comment 58 Caio Lima 2017-04-20 21:40:09 PDT
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.
Comment 59 Caio Lima 2017-04-20 22:15:27 PDT
Created attachment 307692 [details]
proposed Patch

Covering Saam's comments.
Comment 60 Build Bot 2017-04-20 22:17:38 PDT
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.
Comment 61 Build Bot 2017-04-20 23:00:51 PDT
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
Comment 62 Caio Lima 2017-04-21 10:10:54 PDT
Created attachment 307745 [details]
proposed Patch

Fixing regressions.
Comment 63 Build Bot 2017-04-21 10:14:04 PDT
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.
Comment 64 Build Bot 2017-04-21 12:01:48 PDT
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
Comment 65 Build Bot 2017-04-21 12:01:49 PDT
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
Comment 66 Caio Lima 2017-04-21 12:21:03 PDT
Created attachment 307772 [details]
proposed Patch

New Patch Rebased.
Comment 67 Build Bot 2017-04-21 12:23:10 PDT
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.
Comment 68 Caio Lima 2017-04-21 12:30:23 PDT
Created attachment 307774 [details]
Proposed Patch

Last Version was the wrong diff.
Comment 69 Build Bot 2017-04-21 12:33:27 PDT
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.
Comment 70 Saam Barati 2017-04-21 14:48:17 PDT
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)
Comment 71 Caio Lima 2017-04-21 15:14:15 PDT
(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.
Comment 72 Caio Lima 2017-04-21 16:21:26 PDT
Created attachment 307828 [details]
Proposed Patch

Possible Patch for landing.
Comment 73 Build Bot 2017-04-21 16:25:04 PDT
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.
Comment 74 Caio Lima 2017-05-02 07:33:22 PDT
Created attachment 308822 [details]
Proposed Patch

Rebased with master and ping for review
Comment 75 Build Bot 2017-05-02 07:37:14 PDT
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.
Comment 76 Saam Barati 2017-05-09 13:38:53 PDT
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.
Comment 77 Caio Lima 2017-05-10 17:40:05 PDT
Created attachment 309662 [details]
proposed Patch

Rebased Patch with master.
Comment 78 Build Bot 2017-05-10 17:43:38 PDT
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.
Comment 79 Build Bot 2017-05-10 20:17:27 PDT
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
Comment 80 Build Bot 2017-05-10 20:17:29 PDT
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
Comment 81 Caio Lima 2017-05-11 08:39:00 PDT
Created attachment 309711 [details]
Patch for landing

Comments fixed. Let's see what EWS thinks about it.
Comment 82 Build Bot 2017-05-11 08:42:03 PDT
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.
Comment 83 Build Bot 2017-05-11 09:47:13 PDT
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.
Comment 84 Build Bot 2017-05-11 09:47:15 PDT
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
Comment 85 Caio Lima 2017-05-11 10:33:51 PDT
Created attachment 309720 [details]
Patch for landing

Fixing regressions
Comment 86 Build Bot 2017-05-11 10:36:57 PDT
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.
Comment 87 Caio Lima 2017-06-02 19:09:13 PDT
Created attachment 311897 [details]
Patch
Comment 88 Build Bot 2017-06-02 19:12:38 PDT
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.
Comment 89 Saam Barati 2017-06-02 19:32:19 PDT
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.
Comment 90 Caio Lima 2017-06-04 13:33:12 PDT
Created attachment 311970 [details]
Patch
Comment 91 Build Bot 2017-06-04 13:34:52 PDT
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.
Comment 92 Caio Lima 2017-06-06 17:54:59 PDT
Created attachment 312148 [details]
Patch
Comment 93 Build Bot 2017-06-06 17:56:32 PDT
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.
Comment 94 Caio Lima 2017-06-25 08:17:03 PDT
Created attachment 313803 [details]
Patch
Comment 95 Build Bot 2017-06-25 08:18:46 PDT
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.