From f3dff41ce04480723f6d5d0bd92d3d384d95b5bf Mon Sep 17 00:00:00 2001 From: lukes Date: Mon, 18 Dec 2017 13:42:08 -0800 Subject: [PATCH] Handle algebraic identities in PeepholeFoldConstants In particular: x*1 -> x x-0 -> x 0-x -> -x x/1 -> x x+0 -> x An upcoming change to the Soy compiler will generate some of these trivial expressions, and while it would be easy to work around on our end it seemed like a reasonable feature for the jscompiler to handle. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=179462284 --- .../jscomp/PeepholeFoldConstants.java | 93 ++++++++++++++----- .../jscomp/PeepholeFoldConstantsTest.java | 32 ++++++- 2 files changed, 103 insertions(+), 22 deletions(-) diff --git a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java index 1b57e0af095..2c8d838808d 100644 --- a/src/com/google/javascript/jscomp/PeepholeFoldConstants.java +++ b/src/com/google/javascript/jscomp/PeepholeFoldConstants.java @@ -740,17 +740,75 @@ private Node performArithmeticOp(Token opType, Node left, Node right) { // to zero so this is a little awkward here. Double lValObj = NodeUtil.getNumberValue(left); - if (lValObj == null) { + Double rValObj = NodeUtil.getNumberValue(right); + // at least one of the two operands must have a value and both must be numeric + if ((lValObj == null && rValObj == null) || !isNumeric(left) || !isNumeric(right)) { return null; } - Double rValObj = NodeUtil.getNumberValue(right); - if (rValObj == null) { + // handle the operations that have algebraic identities, since we can simplify the tree without + // actually knowing the value statically. + switch (opType) { + case ADD: + if (lValObj != null && rValObj != null) { + return maybeReplaceBinaryOpWithNumericResult(lValObj + rValObj, lValObj, rValObj); + } + if (lValObj != null && lValObj == 0) { + return right.cloneTree(true); + } else if (rValObj != null && rValObj == 0) { + return left.cloneTree(true); + } + return null; + case SUB: + if (lValObj != null && rValObj != null) { + return maybeReplaceBinaryOpWithNumericResult(lValObj - rValObj, lValObj, rValObj); + } + if (lValObj != null && lValObj == 0) { + // 0 - x -> -x + return IR.neg(right.cloneTree(true)); + } else if (rValObj != null && rValObj == 0) { + // x - 0 -> x + return left.cloneTree(true); + } + return null; + case MUL: + if (lValObj != null && rValObj != null) { + return maybeReplaceBinaryOpWithNumericResult(lValObj * rValObj, lValObj, rValObj); + } + // NOTE: 0*x != 0 for all x, if x==0, then it is NaN. So we can't take advantage of that + // without some kind of non-NaN proof. So the special cases here only deal with 1*x + if (lValObj != null) { + if (lValObj == 1) { + return right.cloneTree(true); + } + } else { + if (rValObj == 1) { + return left.cloneTree(true); + } + } + return null; + case DIV: + if (lValObj != null && rValObj != null) { + if (rValObj == 0) { + return null; + } + return maybeReplaceBinaryOpWithNumericResult(lValObj / rValObj, lValObj, rValObj); + } + // NOTE: 0/x != 0 for all x, if x==0, then it is NaN + if (rValObj != null) { + if (rValObj == 1) { + // x/1->x + return left.cloneTree(true); + } + } + return null; + default: + // fall-through + } + if (lValObj == null || rValObj == null) { return null; } - double lval = lValObj; double rval = rValObj; - switch (opType) { case BITAND: result = NodeUtil.toInt32(lval) & NodeUtil.toInt32(rval); @@ -761,31 +819,24 @@ private Node performArithmeticOp(Token opType, Node left, Node right) { case BITXOR: result = NodeUtil.toInt32(lval) ^ NodeUtil.toInt32(rval); break; - case ADD: - result = lval + rval; - break; - case SUB: - result = lval - rval; - break; - case MUL: - result = lval * rval; - break; case MOD: if (rval == 0) { return null; } result = lval % rval; break; - case DIV: - if (rval == 0) { - return null; - } - result = lval / rval; - break; default: - throw new Error("Unexpected arithmetic operator"); + throw new Error("Unexpected arithmetic operator: " + opType); } + return maybeReplaceBinaryOpWithNumericResult(result, lval, rval); + } + + private boolean isNumeric(Node n) { + return NodeUtil.isNumericResult(n) + || (shouldUseTypes && n.getTypeI() != null && n.getTypeI().isNumberValueType()); + } + private Node maybeReplaceBinaryOpWithNumericResult(double result, double lval, double rval) { // TODO(johnlenz): consider removing the result length check. // length of the left and right value plus 1 byte for the operator. if ((String.valueOf(result).length() <= diff --git a/test/com/google/javascript/jscomp/PeepholeFoldConstantsTest.java b/test/com/google/javascript/jscomp/PeepholeFoldConstantsTest.java index 122cee32339..a65be7c2db2 100644 --- a/test/com/google/javascript/jscomp/PeepholeFoldConstantsTest.java +++ b/test/com/google/javascript/jscomp/PeepholeFoldConstantsTest.java @@ -1199,7 +1199,7 @@ public void testFoldLeftChildOp() { foldSame("1 + x - 2 + 3"); foldSame("1 + x - 2 + 3 - 1"); foldSame("f(x)-0"); - foldSame("x-0-0"); + fold("x-0-0", "x-0"); foldSame("x+2-2+2"); foldSame("x+2-2+2-2"); foldSame("x-2+2"); @@ -1507,6 +1507,36 @@ public void testConvertToNumberNegativeInf() { foldSame("var x = 3 * (r ? Infinity : -Infinity);"); } + public void testAlgebraicIdentities() { + this.mode = TypeInferenceMode.BOTH; + + foldNumericTypes("x+0", "x"); + foldNumericTypes("0+x", "x"); + foldNumericTypes("x+0+0+x+x+0", "x+x+x"); + + foldNumericTypes("x-0", "x"); + foldNumericTypes("x-0-0-0", "x"); + // 'x-0' is numeric even if x isn't + fold("var x='a'; x-0-0", "var x='a';x-0"); + foldNumericTypes("0-x", "-x"); + fold("for (var i = 0; i < 5; i++) var x = 0 + i * 1", "for(var i=0; i < 5; i++) var x=i"); + + foldNumericTypes("x*1", "x"); + foldNumericTypes("1*x", "x"); + // can't optimize these without a non-NaN prover + foldSame("x*0"); + foldSame("0*x"); + foldSame("0/x"); + + foldNumericTypes("x/1", "x"); + } + + private void foldNumericTypes(String js, String expected) { + fold( + "function f(/** @type {number} */ x) { " + js + " }", + "function f(/** @type {number} */ x) { " + expected + " }"); + } + private static String join(String operandA, String op, String operandB) { return operandA + " " + op + " " + operandB; }