WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
160012
[DFG] Support ArithPow(Untyped, Untyped)
https://bugs.webkit.org/show_bug.cgi?id=160012
Summary
[DFG] Support ArithPow(Untyped, Untyped)
Yusuke Suzuki
Reported
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.
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
saam
: 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
mjs
: review-
Details
Formatted Diff
Diff
Show Obsolete
(38)
View All
Add attachment
proposed patch, testcase, etc.
Caio Lima
Comment 1
2016-07-25 00:05:32 PDT
Created
attachment 284465
[details]
WIP Patch RFC in this implementation.
Saam Barati
Comment 2
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
Caio Lima
Comment 3
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".
Caio Lima
Comment 4
2016-07-28 00:27:23 PDT
Created
attachment 284757
[details]
Patch
Caio Lima
Comment 5
2016-07-28 00:34:50 PDT
Perf analysis is coming soon.
Caio Lima
Comment 6
2016-07-28 01:07:49 PDT
Created
attachment 284760
[details]
Patch
Keith Miller
Comment 7
2016-07-28 09:17:55 PDT
Looks like ews is sad.
Caio Lima
Comment 8
2016-07-28 09:35:58 PDT
(In reply to
comment #7
)
> Looks like ews is sad.
I think I need rebase the code.
Caio Lima
Comment 9
2016-07-28 10:30:03 PDT
Comment on
attachment 284760
[details]
Patch It is not working properly.
Caio Lima
Comment 10
2016-07-29 03:04:30 PDT
Created
attachment 284857
[details]
Patch
WebKit Commit Bot
Comment 11
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.
Caio Lima
Comment 12
2016-07-29 10:27:12 PDT
Created
attachment 284870
[details]
Patch
WebKit Commit Bot
Comment 13
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.
Caio Lima
Comment 14
2016-07-31 17:01:37 PDT
Created
attachment 284973
[details]
Benchmark run 01
Caio Lima
Comment 15
2016-07-31 17:02:06 PDT
Created
attachment 284974
[details]
Benchmark run 02
Caio Lima
Comment 16
2016-07-31 17:02:31 PDT
Created
attachment 284975
[details]
Benchmark run 03
Caio Lima
Comment 17
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?
Saam Barati
Comment 18
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.
Caio Lima
Comment 19
2016-08-03 10:47:33 PDT
Created
attachment 285246
[details]
Patch
WebKit Commit Bot
Comment 20
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.
Saam Barati
Comment 21
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.
Caio Lima
Comment 22
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
Caio Lima
Comment 23
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.
Caio Lima
Comment 24
2016-08-05 00:02:22 PDT
Created
attachment 285406
[details]
Patch
WebKit Commit Bot
Comment 25
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.
Caio Lima
Comment 26
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
).
Saam Barati
Comment 27
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?
Caio Lima
Comment 28
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.
Caio Lima
Comment 29
2016-08-13 15:07:39 PDT
Created
attachment 286007
[details]
PatchToBot
Caio Lima
Comment 30
2016-08-13 15:20:34 PDT
Created
attachment 286008
[details]
Patch
WebKit Commit Bot
Comment 31
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.
Caio Lima
Comment 32
2017-02-03 17:57:38 PST
Created
attachment 300584
[details]
Patch
Caio Lima
Comment 33
2017-02-03 17:58:46 PST
Let's see what EWS thinks about this Patch.
WebKit Commit Bot
Comment 34
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.
Caio Lima
Comment 35
2017-02-04 10:08:52 PST
Created
attachment 300631
[details]
Patch
Caio Lima
Comment 36
2017-02-04 12:46:39 PST
Created
attachment 300636
[details]
Patch Updated Rebased with upstream/master. Hope it builds now.
WebKit Commit Bot
Comment 37
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.
Caio Lima
Comment 38
2017-02-04 15:14:30 PST
Created
attachment 300640
[details]
Patch Updated
WebKit Commit Bot
Comment 39
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.
Caio Lima
Comment 40
2017-02-17 16:53:31 PST
Created
attachment 302013
[details]
Patch
Caio Lima
Comment 41
2017-02-17 16:54:22 PST
Rebased with master
WebKit Commit Bot
Comment 42
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.
Saam Barati
Comment 43
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
Caio Lima
Comment 44
2017-03-05 17:37:09 PST
Created
attachment 303489
[details]
Patch
WebKit Commit Bot
Comment 45
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.
Caio Lima
Comment 46
2017-03-17 05:28:33 PDT
Created
attachment 304771
[details]
Patch
WebKit Commit Bot
Comment 47
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.
Caio Lima
Comment 48
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.
Build Bot
Comment 49
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.
Caio Lima
Comment 50
2017-03-27 06:53:37 PDT
Ping review
Caio Lima
Comment 51
2017-04-05 09:28:19 PDT
Created
attachment 306289
[details]
Patch fixed Rebasing Patch
Build Bot
Comment 52
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.
Caio Lima
Comment 53
2017-04-05 17:44:30 PDT
Created
attachment 306346
[details]
Fixed Patch Fixing last Patch compilation error.
Build Bot
Comment 54
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.
Caio Lima
Comment 55
2017-04-14 18:01:46 PDT
Created
attachment 307175
[details]
Proposed Patch Fixing some NITs and on bug found into IC implementation.
Build Bot
Comment 56
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.
Saam Barati
Comment 57
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.
Caio Lima
Comment 58
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.
Caio Lima
Comment 59
2017-04-20 22:15:27 PDT
Created
attachment 307692
[details]
proposed Patch Covering Saam's comments.
Build Bot
Comment 60
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.
Build Bot
Comment 61
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
Caio Lima
Comment 62
2017-04-21 10:10:54 PDT
Created
attachment 307745
[details]
proposed Patch Fixing regressions.
Build Bot
Comment 63
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.
Build Bot
Comment 64
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
Build Bot
Comment 65
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
Caio Lima
Comment 66
2017-04-21 12:21:03 PDT
Created
attachment 307772
[details]
proposed Patch New Patch Rebased.
Build Bot
Comment 67
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.
Caio Lima
Comment 68
2017-04-21 12:30:23 PDT
Created
attachment 307774
[details]
Proposed Patch Last Version was the wrong diff.
Build Bot
Comment 69
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.
Saam Barati
Comment 70
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)
Caio Lima
Comment 71
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.
Caio Lima
Comment 72
2017-04-21 16:21:26 PDT
Created
attachment 307828
[details]
Proposed Patch Possible Patch for landing.
Build Bot
Comment 73
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.
Caio Lima
Comment 74
2017-05-02 07:33:22 PDT
Created
attachment 308822
[details]
Proposed Patch Rebased with master and ping for review
Build Bot
Comment 75
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.
Saam Barati
Comment 76
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.
Caio Lima
Comment 77
2017-05-10 17:40:05 PDT
Created
attachment 309662
[details]
proposed Patch Rebased Patch with master.
Build Bot
Comment 78
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.
Build Bot
Comment 79
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
Build Bot
Comment 80
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
Caio Lima
Comment 81
2017-05-11 08:39:00 PDT
Created
attachment 309711
[details]
Patch for landing Comments fixed. Let's see what EWS thinks about it.
Build Bot
Comment 82
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.
Build Bot
Comment 83
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.
Build Bot
Comment 84
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
Caio Lima
Comment 85
2017-05-11 10:33:51 PDT
Created
attachment 309720
[details]
Patch for landing Fixing regressions
Build Bot
Comment 86
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.
Caio Lima
Comment 87
2017-06-02 19:09:13 PDT
Created
attachment 311897
[details]
Patch
Build Bot
Comment 88
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.
Saam Barati
Comment 89
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.
Caio Lima
Comment 90
2017-06-04 13:33:12 PDT
Created
attachment 311970
[details]
Patch
Build Bot
Comment 91
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.
Caio Lima
Comment 92
2017-06-06 17:54:59 PDT
Created
attachment 312148
[details]
Patch
Build Bot
Comment 93
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.
Caio Lima
Comment 94
2017-06-25 08:17:03 PDT
Created
attachment 313803
[details]
Patch
Build Bot
Comment 95
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.
Maciej Stachowiak
Comment 96
2020-05-30 19:29:56 PDT
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
Comment 97
2020-06-01 02:29:03 PDT
This bug isn't valid anymore. We now support ValuePow(UntypedUse) because of BigInt.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug