Now, ** operator is supported. So it is consistent that we support ArithPow(Untyped, Untyped), as similar as ArithSub(Untyped, Untyped), ArithMul(Untyped, Untyped) etc.
Created attachment 284465 [details] WIP Patch RFC in this implementation.
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 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".
Created attachment 284757 [details] Patch
Perf analysis is coming soon.
Created attachment 284760 [details] Patch
Looks like ews is sad.
(In reply to comment #7) > Looks like ews is sad. I think I need rebase the code.
Comment on attachment 284760 [details] Patch It is not working properly.
Created attachment 284857 [details] Patch
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.
Created attachment 284870 [details] Patch
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.
Created attachment 284973 [details] Benchmark run 01
Created attachment 284974 [details] Benchmark run 02
Created attachment 284975 [details] Benchmark run 03
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 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.
Created attachment 285246 [details] Patch
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 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.
Created attachment 285304 [details] New Benchmark Now it is ok. Other benchmarks were dirty. I ran them with --outer 10
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.
Created attachment 285406 [details] Patch
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.
(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 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?
(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.
Created attachment 286007 [details] PatchToBot
Created attachment 286008 [details] Patch
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.
Created attachment 300584 [details] Patch
Let's see what EWS thinks about this Patch.
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.
Created attachment 300631 [details] Patch
Created attachment 300636 [details] Patch Updated Rebased with upstream/master. Hope it builds now.
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.
Created attachment 300640 [details] Patch Updated
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.
Created attachment 302013 [details] Patch
Rebased with master
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 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
Created attachment 303489 [details] Patch
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.
Created attachment 304771 [details] Patch
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.
Created attachment 305275 [details] Patch new proposal This Patch I rebased the code and also fixed some NITs.
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.
Ping review
Created attachment 306289 [details] Patch fixed Rebasing Patch
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.
Created attachment 306346 [details] Fixed Patch Fixing last Patch compilation error.
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.
Created attachment 307175 [details] Proposed Patch Fixing some NITs and on bug found into IC implementation.
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 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 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.
Created attachment 307692 [details] proposed Patch Covering Saam's comments.
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 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
Created attachment 307745 [details] proposed Patch Fixing regressions.
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 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
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
Created attachment 307772 [details] proposed Patch New Patch Rebased.
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.
Created attachment 307774 [details] Proposed Patch Last Version was the wrong diff.
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 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)
(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.
Created attachment 307828 [details] Proposed Patch Possible Patch for landing.
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.
Created attachment 308822 [details] Proposed Patch Rebased with master and ping for review
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 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.
Created attachment 309662 [details] proposed Patch Rebased Patch with master.
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 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
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
Created attachment 309711 [details] Patch for landing Comments fixed. Let's see what EWS thinks about it.
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 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.
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
Created attachment 309720 [details] Patch for landing Fixing regressions
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.
Created attachment 311897 [details] Patch
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 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.
Created attachment 311970 [details] Patch
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.
Created attachment 312148 [details] Patch
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.
Created attachment 313803 [details] Patch
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.
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.
This bug isn't valid anymore. We now support ValuePow(UntypedUse) because of BigInt.