Add a DFG node for the Pow Intrinsics
Created attachment 246481 [details] Patch
Attachment 246481 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:726: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246481 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246481&action=review I think this is pretty good! You have a register allocation bug, and I your prediction propagation and abstract interpretation might be a bit unrefined, but it's very close to perfect. > Source/JavaScriptCore/ChangeLog:12 > + With this patch I get the following changes on benchmarks: I would call out that by changes you mean progressions. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:726 > + if (childY.asNumber() == 0) { !childY.asNumber() :-) I never liked this style, but we do follow it, so we might as well keep it up. > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:737 > + forNode(node).setType(SpecFullDouble); Will it ever create signaling NaN? Seems like it won't. Maybe this should be SpecDoubleReal | SpecDoublePureNaN. I recommend using the style of other arithmetics, see things like typeOfDoubleSum in SpeculatedType.h|cpp. It's fine if this is a dumb stub for now that returns SpecDoubleReal | SpecDoublePureNaN. The reason why this is useful is that we have to purify NaN on store into typed arrays. So having a proof that pow doesn't return signaling NaN would be important. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1860 > - > + Revert! > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:343 > + if (node->child1()->shouldSpeculateNumber() && node->child2()->shouldSpeculateInt32()) { Why not just "if (node->child2()->shouldSpeculateInt32OrBooleanForArithmetic())"? The check for child1 is not meaningful. As written, this means that if the first child had some non-numeric stuff sometimes flowing into it then it would prevent integer speculation for the second child. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:344 > + fixEdge<DoubleRepUse>(node->child1()); Why not fixDoubleOrBooleanEdge? > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:345 > + fixEdge<Int32Use>(node->child2()); Why not fixIntOrBooleanEdge? > Source/JavaScriptCore/dfg/DFGNode.h:573 > + > + void convertToArithSqrt() > + { > + ASSERT(m_op == ArithPow); > + m_op = ArithSqrt; > + } Ordinarily conversion nodes that involve having different children will shuffle the children around or clear them appropriately. So, this should probably also do: child2() = Edge(); > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:329 > + // FIXME: is that right? The native pow() can return an impoure NaN. > + // But so can sqrt() above and it's type is SpecBytecodeDouble, what am I missing. > + changed |= setPrediction(SpecFullDouble); > + break; If the native pow() can return impure NaN then maybe we should have the call to pow() do the NaN sanitization. This would make sense because the fast path of pow() uses your integer exponent math, which cannot create an impure NaN. Does pow sometimes return signaling NaN even if passed non-signaling NaN or real inputs? Can sqrt() return impure NaN if it is passed a real number or pure NaN as input? Or is it just that it returns impure NaN if passed impure NaN? In AbstractInterpreter, ArithSqrt uses typeOfDoubleUnaryOp() which I believe does the right things: returns pure NaN or real if the input is pure NaN or real, but may pass through impure NaN. PredictionPropagation should probably also call the same thing, but it doesn't really matter. It's OK for PredictionPropagation to underestimate the set of types returned by a node. PredictionPropagation does not need to be sound, for the compiler to generate sound code. The predicted types deduced here are used only to decide how to speculate. Worst case is that someone might choose to speculate that the output of ArithSqrt is never an impure NaN because PredictionPropagation says that it's always a bytecode double. To my knowledge, this won't actually happen, because we never speculate that something is not impure NaN (though we do speculate that something is not NaN, which is a different thing). > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:339 > - > + Revert! > Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:790 > - > + Revert! > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3584 > + if (!m_jit.supportsFloatingPoint()) { > + SpeculateDoubleOperand xOperand(this, node->child1()); > + SpeculateDoubleOperand yOperand(this, node->child2()); > + FPRReg xOperandfpr = xOperand.fpr(); > + FPRReg yOperandfpr = yOperand.fpr(); > + > + FPRTemporary result(this); > + callOperation(operationMathPow, result.fpr(), xOperandfpr, yOperandfpr); > + doubleResult(result.fpr(), node); > + return; > + } > + This is dead code. We don't go into the DFG if floating point isn't supported. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3604 > + callOperation(operationMathPow, result.fpr(), xOperandfpr, yOperandfpr.fpr()); How rare is this slow path? If it's less than 10% likely, then instead of flushing registers above, you should put this inside a slow path generator. That will put the call out-of-line and it will put spilling/refilling around the call, so that we don't spill everything on the fast path. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3626 > + m_jit.branchConvertDoubleToInt32(yOperandfpr, yOperandInteger.gpr(), failedExponentConversionToInteger, scratch.fpr(), false); > + > + m_jit.moveDouble(xOperandfpr, xOperandCopy.fpr()); Two comments: 1) Super dangerous to call gpr() and fpr() after you've already started doing control flow without having first called them prior to that control flow. Both methods may invoke register allocation fill/spill routines. Those routines expect to be generated in straight-line code, not inside code that is guarded by control flow. You should just make sure that for all of those temporaries, you get their fprs/gprs before doing anything else. 2) Did you consider doing the >=1000 check merged with the conversion-succeeded check? Seems like it would be worthwhile to do that as part of this patch, if we are to do it at all. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3630 > + callOperation(operationMathPow, result.fpr(), xOperandfpr, yOperandfpr); Ditto about putting this in a slow path generator, if you think that this will be taken less than 1/10 of the time. > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:139 > + if (yOperandValue == 1) { I'm surprised you didn't do Math.pow(x, -1) as well. :-) Is that feasible? > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:143 > + if (MacroAssembler::supportsFloatingPointSqrt()) { As I mentioned before, this condition doesn't make sense. Consider what happens on a machine that doesn't support sqrt() directly. Either the sqrt() function is faster than the pow() function, or it's not. If it is, then of course we still want to do this optimization. If it's not, then we want ArithSqrt implemented using a call to pow(), in which case it would still make sense to do this optimization. It would then be a canonicalization and not an optimization, but still, we would want to do it. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1640 > + LValue oneBaseInfiniteExponent = m_out.bitAnd(absoluteExponentIsInfinity, absoluteBaseIsOne); Don't do this. LLVM generates very bad code for bitAnd over conditionals. You should split this into two branches and an extra basic block in between. Oddly, LLVM will sometimes convert such a control flow structure into code using bitAnd anyway, but we are loudly lobbying for this "optimization" to be ripped out because it almost always causes a slow-down. > Source/JavaScriptCore/jit/JITOperations.h:343 > + Revert! > Source/JavaScriptCore/tests/stress/math-pow-basics.js:157 > +function test6(x, y, expected1, expected2) { > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble6(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble6(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleInt6(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleInt6(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; > + } > +} > +noInline(test6); ~10000 executions of a straight-line noInline function should be enough! Our no-cjit test runs are run with thresholds explicitly set to guarantee that 10000 always hits the FTL. > Source/JavaScriptCore/tests/stress/math-pow-basics.js:180 > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble7(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble7(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble7(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleDouble7(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-basics.js:206 > +function test8(x, y, expected1, expected2) { > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble8(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble8(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleInt8(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleInt8(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-basics.js:232 > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble9(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble9(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleInt9(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleInt9(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; > + } Ditto. > Source/JavaScriptCore/tests/stress/math-pow-basics.js:258 > +function test10(x, y, expected1, expected2) { > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble10(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleInt10(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleInt(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; > + } Ditto. > Source/JavaScriptCore/tests/stress/math-pow-basics.js:282 > +function test11(x, y, expected1, expected2) { > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDouble11(x, y); > + if (!valuesAreClose(result, expected1)) > + throw "Error: bad result, mathPowDoubleDouble(" + x + ", " + y + ") = " + result + " expected a value close to " + expected1; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleInt11(x, integerY); > + if (!valuesAreClose(result, expected2)) > + throw "Error: bad result, mathPowDoubleInt(" + x + ", " + integerY + ") = " + result + " expected a value close to " + expected2; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-integer-exponent-fastpath.js:27 > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDoubleTestExponentFifty(x, y); > + if (!valuesAreClose(result, expected)) > + throw "Error: bad result, Math.pow(" + x + ", " + y + ") = " + result + " expected value close to " + expected; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleIntTestExponentFifty(x, integerY); > + if (!valuesAreClose(result, expected)) > + throw "Error: bad result, Math.pow(" + x + ", " + integerY + ") = " + result + " expected value close to " + expected; > + } > +} Ditto. > Source/JavaScriptCore/tests/stress/math-pow-integer-exponent-fastpath.js:51 > +function testExponentTenThousands(x, y, expected) { > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleDoubleTestExponentTenThousands(x, y); > + if (!valuesAreClose(result, expected)) > + throw "Error: bad result, Math.pow(" + x + ", " + y + ") = " + result + " expected value close to " + expected; > + } > + var integerY = y | 0; > + for (var i = 0; i < 1000000; ++i) { > + var result = mathPowDoubleIntTestExponentTenThousands(x, integerY); > + if (!valuesAreClose(result, expected)) > + throw "Error: bad result, Math.pow(" + x + ", " + integerY + ") = " + result + " expected value close to " + expected; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-nan-behaviors.js:11 > +function testIntegerBaseWithNaNExponent() { > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(5, NaN); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(5, NaN) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(i, NaN); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(i, NaN) = " + result + " with i = " + i; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-nan-behaviors.js:27 > +function testFloatingPointBaseWithNaNExponent() { > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(5.5, NaN); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(5.5, NaN) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(i + 1, NaN); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(i + 0.5, NaN) = " + result + " with i = " + i; > + } Ditto. > Source/JavaScriptCore/tests/stress/math-pow-nan-behaviors.js:56 > +function testNaNBase() { > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(NaN, i + 1); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(NaN, i + 1) = " + result + " with i = " + i; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(NaN, i + 1.5); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(NaN, i + 1.5) = " + result + " with i = " + i; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(NaN, 0); > + if (result !== 1) > + throw "Error: bad result, Math.pow(NaN, 0) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(NaN, -0); > + if (result !== 1) > + throw "Error: bad result, Math.pow(NaN, -0) = " + result; > + } > +} Ditto. > Source/JavaScriptCore/tests/stress/math-pow-nan-behaviors.js:82 > +function infiniteExponents() { > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(1, Number.POSITIVE_INFINITY); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(1, Number.POSITIVE_INFINITY) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(-1, Number.POSITIVE_INFINITY); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(-1, Number.POSITIVE_INFINITY) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(1, Number.NEGATIVE_INFINITY); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(1, Number.NEGATIVE_INFINITY) = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = Math.pow(-1, Number.NEGATIVE_INFINITY); > + if (!isNaN(result)) > + throw "Error: bad result, Math.pow(-1, Number.NEGATIVE_INFINITY) = " + result; > + } #document > Source/JavaScriptCore/tests/stress/math-pow-with-constants.js:15 > + for (var i = 0; i < 1000000; ++i) { > + var result = exponentIsZero(5); > + if (result !== 1) > + throw "Error: zeroExponent(5) should be 1, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = exponentIsZero(5.5); > + if (result !== 1) > + throw "Error: zeroExponent(5.5) should be 1, was = " + result; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-with-constants.js:34 > +function testExponentIsOne() { > + for (var i = 0; i < 1000000; ++i) { > + var result = exponentIsOne(5); > + if (result !== 5) > + throw "Error: exponentIsOne(5) should be 5, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = exponentIsOne(5.5); > + if (result !== 5.5) Ditto. > Source/JavaScriptCore/tests/stress/math-pow-with-constants.js:55 > +function testPowUsedAsSqrt() { > + for (var i = 0; i < 1000000; ++i) { > + var result = powUsedAsSqrt(4); > + if (result !== Math.sqrt(4)) > + throw "Error: powUsedAsSqrt(4) should be 2, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = powUsedAsSqrt(4.4); > + if (result !== Math.sqrt(4.4)) > throw "Error: powUsedAsSqrt(4) should be " + Math.sqrt(4.4) + ", was = " + result; Ditto. > Source/JavaScriptCore/tests/stress/math-pow-with-constants.js:113 > +{ > + for (var i = 0; i < 1000000; ++i) { > + var result = intIntConstantsSmallNumbers(); > + if (result !== 74088) > + throw "Error: intIntConstantsSmallNumbers() should be 74088, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = intIntConstantsLargeNumbers(); > + if (result !== 1.5013093754529656e+68) > + throw "Error: intIntConstantsLargeNumbers() should be 1.5013093754529656e+68, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = intDoubleConstants(); > + if (result !== 5.1338303882015765e+48) > + throw "Error: intDoubleConstants() should be 5.1338303882015765e+48, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = doubleDoubleConstants(); > + if (result !== 1.0944228729647829e+48) > + throw "Error: doubleDoubleConstants() should be 1.0944228729647829e+48, was = " + result; > + } > + for (var i = 0; i < 1000000; ++i) { > + var result = doubleIntConstants(); > + if (result !== 5.989022735311158e+58) > + throw "Error: doubleIntConstants() should be 5.989022735311158e+58, was = " + result; > + } Ditto.
Created attachment 246560 [details] Patch
Comment on attachment 246560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246560&action=review Almost there! > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:737 > + forNode(node).setType(typeOfDoubleUnaryOp(forNode(node->child1()).m_type)); I think you want typeOfDoubleBinaryOp and pass the types of both inputs. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:343 > + if (node->child2()->shouldSpeculateInt32()) { shouldSpeculateInt32OrBooleanForArithmetic :-)
Created attachment 246564 [details] Patch
Created attachment 246567 [details] Patch
Created attachment 246568 [details] Patch
Hopefully I got the type right this time: 1) If the exponent is any NaN, we know that the result is a pure NaN. 2) Otherwise, business as usual. SpeculatedType typeOfDoublePow(SpeculatedType xValue, SpeculatedType yValue) { // Math.pow() always return NaN if the exponent is NaN, unlike std::pow(). // We always set a pure NaN in that case. if (yValue & SpecDoubleNaN) xValue |= SpecDoublePureNaN; return polluteDouble(xValue); }
Attachment 246568 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1294: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1295: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1296: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1329: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1331: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1331: Extra space between int and k [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1335: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1335: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1336: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1336: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1336: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1340: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1342: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1342: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1344: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1344: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1346: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1346: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1349: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1350: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1350: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1353: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1359: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1360: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1361: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1362: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1363: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1365: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1365: Missing spaces around = [whitespace/operators] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1366: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1367: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1368: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1371: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1371: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1371: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1371: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1374: 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/jit/JITOperations.cpp:1374: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1374: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1374: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1375: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1375: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1375: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1384: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1384: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1385: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1385: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1385: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1386: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1386: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1388: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1390: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1390: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1390: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1391: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1391: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1391: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1393: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1393: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1393: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1399: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1399: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1399: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1400: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1401: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1401: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1401: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1402: Extra space between return and y [whitespace/declaration] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1404: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1406: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1408: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1408: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1409: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1409: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1409: Missing spaces around / [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1409: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1409: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1410: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1411: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1411: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1411: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1412: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1412: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1413: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1413: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1420: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1420: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1420: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1421: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1421: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1421: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1421: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1423: Missing spaces around / [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1423: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1423: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1424: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1424: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1425: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1425: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1425: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1426: Missing spaces around / [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1427: Missing spaces around == [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1427: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1437: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1437: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1437: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1437: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1440: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1440: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1440: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1440: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1443: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1444: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1444: Missing space before { [whitespace/braces] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1445: Missing spaces around <= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1445: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1445: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1446: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1446: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1446: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1449: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1449: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1449: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1450: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1450: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1461: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1461: s_l is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1464: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1464: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1465: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1469: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp :1470: Missing spaces around = [whitespace/operators] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1470: Missing spaces around <= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1470: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1470: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1471: Missing spaces around = [whitespace/operators] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1471: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1471: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1471: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1472: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1472: Missing spaces around = [whitespace/operators] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1472: Missing spaces around += [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1477: Missing spaces around / [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1483: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1518: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1519: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1519: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1519: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1522: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1522: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1523: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1524: Missing spaces around >= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1524: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1524: Extra space before ) in if [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1525: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1525: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1525: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1528: Missing spaces around <= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1528: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1528: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1529: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1537: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1542: Missing spaces around | [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1543: Missing spaces around < [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1543: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1543: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1554: Missing spaces around / [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1558: Missing spaces around <= [whitespace/operators] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1558: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1558: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1558: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/JITOperations.cpp:1559: Else clause should never be on same line as else (use 2 lines) [whitespace/newline] [4] Total errors found: 172 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246568&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:1633 > + // FIXME: shouldn't we only check that if the type of child2() might have NaN? Yup. :-) An idiom that I like to use for this is: LValue exponentIsNaN; if (forNode(child2).m_type & SpecDoubleNaN) exponentIsNaN = m_out.doubleNotEqualOrUnordered(exponent, exponent); else exponentIsNaN = m_out.booleanFalse; You don't have to do this in this patch. > Source/JavaScriptCore/jit/JITOperations.cpp:1238 > +#if PLATFORM(IOS) && CPU(ARM_THUMB2) > + > +// The following code is taken from netlib.org: > +// http://www.netlib.org/fdlibm/fdlibm.h > +// http://www.netlib.org/fdlibm/e_pow.c > +// http://www.netlib.org/fdlibm/s_scalbn.c > +// > +// And was originally distributed under the following license: > + > +/* > + * ==================================================== > + * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved. > + * > + * Developed at SunSoft, a Sun Microsystems, Inc. business. > + * Permission to use, copy, modify, and distribute this > + * software is freely granted, provided that this notice > + * is preserved. > + * ==================================================== > + */ > +/* > + * ==================================================== > + * Copyright (C) 2004 by Sun Microsystems, Inc. All rights reserved. > + * It really feels like this code should belong in its own directory, like even an fdlibm directory. Maybe we should do that in a future patch.
Comment on attachment 246568 [details] Patch Clearing flags on attachment: 246568 Committed r180098: <http://trac.webkit.org/changeset/180098>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > Comment on attachment 246568 [details] > Patch > > Clearing flags on attachment: 246568 > > Committed r180098: <http://trac.webkit.org/changeset/180098> It broke the cloop build.
just a note: new bug report for buildfix - bug141618
*** Bug 118560 has been marked as a duplicate of this bug. ***