# HG changeset patch
# User hannesw
# Date 1382689249 -7200
# Node ID 7985ec3782b55e53e13abd833a6d77003603cbe8
# Parent  f31ee3a2847de99231c6ff5451c37b8462af2602
8027042: Evaluation order for binary operators can be improved
Reviewed-by: lagergren, jlaskey, attila

diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/codegen/CodeGenerator.java
--- a/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/codegen/CodeGenerator.java	Fri Oct 25 10:20:49 2013 +0200
@@ -359,8 +359,11 @@
         return load(node, node.hasType() ? node.getType() : null, false);
     }
 
-    private static boolean safeLiteral(final Expression rhs) {
-        return rhs instanceof LiteralNode && !(rhs instanceof ArrayLiteralNode);
+    // Test whether conversion from source to target involves a call of ES 9.1 ToPrimitive
+    // with possible side effects from calling an object's toString or valueOf methods.
+    private boolean noToPrimitiveConversion(final Type source, final Type target) {
+        // Object to boolean conversion does not cause ToPrimitive call
+        return source.isJSPrimitive() || !target.isJSPrimitive() || target.isBoolean();
     }
 
     MethodEmitter loadBinaryOperands(final Expression lhs, final Expression rhs, final Type type) {
@@ -374,25 +377,19 @@
         // can combine a LOAD with a CONVERT operation (e.g. use a dynamic getter with the conversion target type as its
         // return value). What we do here is reorder LOAD RIGHT and CONVERT LEFT when possible; it is possible only when
         // we can prove that executing CONVERT LEFT can't have a side effect that changes the value of LOAD RIGHT.
-        // Basically, if we know that either LEFT is not an object, or RIGHT is a constant literal, then we can do the
+        // Basically, if we know that either LEFT already is a primitive value, or does not have to be converted to
+        // a primitive value, or RIGHT is an expression that loads without side effects, then we can do the
         // reordering and collapse LOAD/CONVERT into a single operation; otherwise we need to do the more costly
         // separate operations to preserve specification semantics.
-        final Type lhsType = lhs.getType();
-        if (lhsType.isObject() && !safeLiteral(rhs)) {
-            // Can't reorder. Load and convert separately.
-            load(lhs, lhsType, baseAlreadyOnStack);
-            load(rhs, rhs.getType(), false);
-            // Avoid empty SWAP, SWAP bytecode sequence if CONVERT LEFT is a no-op
-            if (!lhsType.isEquivalentTo(type)) {
-                method.swap();
-                method.convert(type);
-                method.swap();
-            }
-            method.convert(type);
-        } else {
+        if (noToPrimitiveConversion(lhs.getType(), type) || rhs.isLocal()) {
             // Can reorder. Combine load and convert into single operations.
             load(lhs, type, baseAlreadyOnStack);
             load(rhs, type, false);
+        } else {
+            // Can't reorder. Load and convert separately.
+            load(lhs, lhs.getType(), baseAlreadyOnStack);
+            load(rhs, rhs.getType(), false);
+            method.swap().convert(type).swap().convert(type);
         }
 
         return method;
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/codegen/types/Type.java
--- a/src/jdk/nashorn/internal/codegen/types/Type.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/codegen/types/Type.java	Fri Oct 25 10:20:49 2013 +0200
@@ -292,6 +292,16 @@
     }
 
     /**
+     * Determines whether this type represents an primitive type according to the ECMAScript specification,
+     * which includes Boolean, Number, and String.
+     *
+     * @return true if a JavaScript primitive type, false otherwise.
+     */
+    public boolean isJSPrimitive() {
+        return !isObject() || isString();
+    }
+
+    /**
      * Determines whether a type is the BOOLEAN type
      * @return true if BOOLEAN, false otherwise
      */
@@ -443,7 +453,7 @@
         } else if (type0.isArray() != type1.isArray()) {
             //array and non array is always object, widest(Object[], int) NEVER returns Object[], which has most weight. that does not make sense
             return Type.OBJECT;
-        } else if (type0.isObject() && type1.isObject() && ((ObjectType)type0).getTypeClass() != ((ObjectType)type1).getTypeClass()) {
+        } else if (type0.isObject() && type1.isObject() && type0.getTypeClass() != type1.getTypeClass()) {
             // Object<type=String> and Object<type=ScriptFunction> will produce Object
             // TODO: maybe find most specific common superclass?
             return Type.OBJECT;
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/BinaryNode.java
--- a/src/jdk/nashorn/internal/ir/BinaryNode.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/BinaryNode.java	Fri Oct 25 10:20:49 2013 +0200
@@ -90,6 +90,9 @@
             return Type.LONG;
         case ASSIGN_SAR:
         case ASSIGN_SHL:
+        case BIT_AND:
+        case BIT_OR:
+        case BIT_XOR:
         case ASSIGN_BIT_AND:
         case ASSIGN_BIT_OR:
         case ASSIGN_BIT_XOR:
@@ -170,6 +173,42 @@
     }
 
     @Override
+    public boolean isLocal() {
+        switch (tokenType()) {
+        case SAR:
+        case SHL:
+        case SHR:
+        case BIT_AND:
+        case BIT_OR:
+        case BIT_XOR:
+        case ADD:
+        case DIV:
+        case MOD:
+        case MUL:
+        case SUB:
+            return lhs.isLocal() && lhs.getType().isJSPrimitive()
+                && rhs.isLocal() && rhs.getType().isJSPrimitive();
+        case ASSIGN_ADD:
+        case ASSIGN_BIT_AND:
+        case ASSIGN_BIT_OR:
+        case ASSIGN_BIT_XOR:
+        case ASSIGN_DIV:
+        case ASSIGN_MOD:
+        case ASSIGN_MUL:
+        case ASSIGN_SAR:
+        case ASSIGN_SHL:
+        case ASSIGN_SHR:
+        case ASSIGN_SUB:
+            return lhs instanceof IdentNode && lhs.isLocal() && lhs.getType().isJSPrimitive()
+                    && rhs.isLocal() && rhs.getType().isJSPrimitive();
+        case ASSIGN:
+            return lhs instanceof IdentNode && lhs.isLocal() && rhs.isLocal();
+        default:
+            return false;
+        }
+    }
+
+    @Override
     public void toString(final StringBuilder sb) {
         final TokenType type = tokenType();
 
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/Expression.java
--- a/src/jdk/nashorn/internal/ir/Expression.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/Expression.java	Fri Oct 25 10:20:49 2013 +0200
@@ -96,4 +96,16 @@
         assert hasType() : this + " has no type";
         return symbol.getSymbolType();
     }
+
+    /**
+     * Returns {@code true} if this expression depends exclusively on state that is constant
+     * or local to the currently running function and thus inaccessible to other functions.
+     * This implies that a local expression must not call any other functions (neither directly
+     * nor implicitly through a getter, setter, or object-to-primitive type conversion).
+     *
+     * @return true if this expression does not depend on state shared with other functions.
+     */
+    public boolean isLocal() {
+        return false;
+    }
 }
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/IdentNode.java
--- a/src/jdk/nashorn/internal/ir/IdentNode.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/IdentNode.java	Fri Oct 25 10:20:49 2013 +0200
@@ -138,6 +138,11 @@
         return getName();
     }
 
+    @Override
+    public boolean isLocal() {
+        return !getSymbol().isScope();
+    }
+
     /**
      * Check if this IdentNode is a property name
      * @return true if this is a property name
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/LiteralNode.java
--- a/src/jdk/nashorn/internal/ir/LiteralNode.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/LiteralNode.java	Fri Oct 25 10:20:49 2013 +0200
@@ -275,6 +275,11 @@
         public boolean isTrue() {
             return JSType.toBoolean(value);
         }
+
+        @Override
+        public boolean isLocal() {
+            return true;
+        }
     }
 
     @Immutable
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/TernaryNode.java
--- a/src/jdk/nashorn/internal/ir/TernaryNode.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/TernaryNode.java	Fri Oct 25 10:20:49 2013 +0200
@@ -109,6 +109,13 @@
         }
     }
 
+    @Override
+    public boolean isLocal() {
+        return getTest().isLocal()
+                && getTrueExpression().isLocal()
+                && getFalseExpression().isLocal();
+    }
+
     /**
      * Get the test expression for this ternary expression, i.e. "x" in x ? y : z
      * @return the test expression
diff -r f31ee3a2847d -r 7985ec3782b5 src/jdk/nashorn/internal/ir/UnaryNode.java
--- a/src/jdk/nashorn/internal/ir/UnaryNode.java	Wed Oct 23 20:15:43 2013 +0530
+++ b/src/jdk/nashorn/internal/ir/UnaryNode.java	Fri Oct 25 10:20:49 2013 +0200
@@ -129,6 +129,26 @@
     }
 
     @Override
+    public boolean isLocal() {
+        switch (tokenType()) {
+            case NEW:
+                return false;
+            case ADD:
+            case SUB:
+            case NOT:
+            case BIT_NOT:
+                return rhs.isLocal() && rhs.getType().isJSPrimitive();
+            case DECPOSTFIX:
+            case DECPREFIX:
+            case INCPOSTFIX:
+            case INCPREFIX:
+                return rhs instanceof IdentNode && rhs.isLocal() && rhs.getType().isJSPrimitive();
+            default:
+                return rhs.isLocal();
+        }
+    }
+
+    @Override
     public void toString(final StringBuilder sb) {
         toString(sb, new Runnable() {
             @Override
diff -r f31ee3a2847d -r 7985ec3782b5 test/script/basic/JDK-8027042.js
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/script/basic/JDK-8027042.js	Fri Oct 25 10:20:49 2013 +0200
@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ * 
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ * 
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ * 
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ * 
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/**
+ * JDK-8027042: Evaluation order for binary operators can be improved
+ *
+ * @test
+ * @run
+ */
+
+// var with getter side effect
+Object.defineProperty(this, "a", { get: function() {print("get a"); return 1; }});
+
+// var with both getter and conversion side effect
+Object.defineProperty(this, "b", { get: function() {print("get b"); return {valueOf: function() { print("conv b"); return 10; }}; }});
+
+(function() {
+    // var with toPrimitive conversion side effect
+    var c = {valueOf: function() { print("conv c"); return 100; }};
+
+    print(b + (c + a));
+    print(b + (c + b));
+    print(b + (a + b));
+    print(b + (b + c));
+    print(b + (b + c));
+    print(b + (c + (a - b)));
+    print(b + (c + (c - b)));
+    print(b + (c + (b - c)));
+    print(b + (b + (a ? 2 : 3)));
+    print(b + (b + (b ? 2 : 3)));
+    print(b + (b + (c ? 2 : 3)));
+    print(b + ((-c) + (-a)));
+    print(b + ((-c) + (-b)));
+    print(b + ((-c) + (-c)));
+    try { print(b + new a); } catch (e) {}
+    try { print(b + new b); } catch (e) {}
+    try { print(b + new c); } catch (e) {}
+})();
diff -r f31ee3a2847d -r 7985ec3782b5 test/script/basic/JDK-8027042.js.EXPECTED
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/script/basic/JDK-8027042.js.EXPECTED	Fri Oct 25 10:20:49 2013 +0200
@@ -0,0 +1,88 @@
+get b
+get a
+conv c
+conv b
+111
+get b
+get b
+conv c
+conv b
+conv b
+120
+get b
+get a
+get b
+conv b
+conv b
+21
+get b
+get b
+conv b
+conv c
+conv b
+120
+get b
+get b
+conv b
+conv c
+conv b
+120
+get b
+get a
+get b
+conv b
+conv c
+conv b
+101
+get b
+get b
+conv c
+conv b
+conv c
+conv b
+200
+get b
+get b
+conv b
+conv c
+conv c
+conv b
+20
+get b
+get b
+get a
+conv b
+conv b
+22
+get b
+get b
+get b
+conv b
+conv b
+22
+get b
+get b
+conv b
+conv b
+22
+get b
+conv c
+get a
+conv b
+-91
+get b
+conv c
+get b
+conv b
+conv b
+-100
+get b
+conv c
+conv c
+conv b
+-190
+get b
+get a
+get b
+get b
+get b