8009170: Regression: javac generates redundant bytecode in assignop involving arrays

Reviewed-by: mcimadamore

src/share/classes/com/sun/tools/javac/comp/Lower.java

@@ -3158,38 +3158,59 @@ public class Lower extends TreeTranslator {
     }
 
     public void visitAssignop(final JCAssignOp tree) {
+        JCTree lhsAccess = access(TreeInfo.skipParens(tree.lhs));
         final boolean boxingReq = !tree.lhs.type.isPrimitive() &&
             tree.operator.type.getReturnType().isPrimitive();
 
-        // boxing required; need to rewrite as x = (unbox typeof x)(x op y);
-        // or if x == (typeof x)z then z = (unbox typeof x)((typeof x)z op y)
-        // (but without recomputing x)
-        JCTree newTree = abstractLval(tree.lhs, new TreeBuilder() {
-                public JCTree build(final JCTree lhs) {
-                    JCTree.Tag newTag = tree.getTag().noAssignOp();
-                    // Erasure (TransTypes) can change the type of
-                    // tree.lhs.  However, we can still get the
-                    // unerased type of tree.lhs as it is stored
-                    // in tree.type in Attr.
-                    Symbol newOperator = rs.resolveBinaryOperator(tree.pos(),
-                                                                  newTag,
-                                                                  attrEnv,
-                                                                  tree.type,
-                                                                  tree.rhs.type);
-                    JCExpression expr = (JCExpression)lhs;
-                    if (expr.type != tree.type)
-                        expr = make.TypeCast(tree.type, expr);
-                    JCBinary opResult = make.Binary(newTag, expr, tree.rhs);
-                    opResult.operator = newOperator;
-                    opResult.type = newOperator.type.getReturnType();
-                    JCExpression newRhs = boxingReq ?
-                            make.TypeCast(types.unboxedType(tree.type),
-                                                      opResult) :
+        if (boxingReq || lhsAccess.hasTag(APPLY)) {
+            // boxing required; need to rewrite as x = (unbox typeof x)(x op y);
+            // or if x == (typeof x)z then z = (unbox typeof x)((typeof x)z op y)
+            // (but without recomputing x)
+            JCTree newTree = abstractLval(tree.lhs, new TreeBuilder() {
+                    public JCTree build(final JCTree lhs) {
+                        JCTree.Tag newTag = tree.getTag().noAssignOp();
+                        // Erasure (TransTypes) can change the type of
+                        // tree.lhs.  However, we can still get the
+                        // unerased type of tree.lhs as it is stored
+                        // in tree.type in Attr.
+                        Symbol newOperator = rs.resolveBinaryOperator(tree.pos(),
+                                                                      newTag,
+                                                                      attrEnv,
+                                                                      tree.type,
+                                                                      tree.rhs.type);
+                        JCExpression expr = (JCExpression)lhs;
+                        if (expr.type != tree.type)
+                            expr = make.TypeCast(tree.type, expr);
+                        JCBinary opResult = make.Binary(newTag, expr, tree.rhs);
+                        opResult.operator = newOperator;
+                        opResult.type = newOperator.type.getReturnType();
+                        JCExpression newRhs = boxingReq ?
+                            make.TypeCast(types.unboxedType(tree.type), opResult) :
                             opResult;
-                    return make.Assign((JCExpression)lhs, newRhs).setType(tree.type);
-                }
-            });
-        result = translate(newTree);
+                        return make.Assign((JCExpression)lhs, newRhs).setType(tree.type);
+                    }
+                });
+            result = translate(newTree);
+            return;
+        }
+        tree.lhs = translate(tree.lhs, tree);
+        tree.rhs = translate(tree.rhs, tree.operator.type.getParameterTypes().tail.head);
+
+        // If translated left hand side is an Apply, we are
+        // seeing an access method invocation. In this case, append
+        // right hand side as last argument of the access method.
+        if (tree.lhs.hasTag(APPLY)) {
+            JCMethodInvocation app = (JCMethodInvocation)tree.lhs;
+            // if operation is a += on strings,
+            // make sure to convert argument to string
+            JCExpression rhs = (((OperatorSymbol)tree.operator).opcode == string_add)
+              ? makeString(tree.rhs)
+              : tree.rhs;
+            app.args = List.of(rhs).prependList(app.args);
+            result = app;
+        } else {
+            result = tree;
+        }
     }
 
     /** Lower a tree of the form e++ or e-- where e is an object type */

test/tools/javac/7167125/DiffResultAfterSameOperationInnerClasses.java

@@ -34,27 +34,60 @@ public class DiffResultAfterSameOperationInnerClasses {
     private int j = 1;
     public String s1 = "Hi, ";
     private String s2 = "Hi, ";
+    public int arr1[] = new int[]{1};
+    public int arr2[] = new int[]{1};
 
     public static void main(String[] args) {
-        InnerClass inner =
-                new DiffResultAfterSameOperationInnerClasses().new InnerClass();
-        if (!inner.test()) {
+        DiffResultAfterSameOperationInnerClasses theTest =
+                new DiffResultAfterSameOperationInnerClasses();
+        InnerClass inner = theTest.new InnerClass();
+        if (!inner.test1()) {
+            throw new AssertionError("Different results after same calculation");
+        }
+
+        theTest.resetVars();
+        if (!inner.test2()) {
             throw new AssertionError("Different results after same calculation");
         }
     }
 
+    void resetVars() {
+        i = 1;
+        j = 1;
+        s1 = "Hi, ";
+        s2 = "Hi, ";
+        arr1[0] = 1;
+        arr2[0] = 1;
+    }
+
     class InnerClass {
-        public boolean test() {
+        public boolean test1() {
             i += i += 1;
             j += j += 1;
 
+            arr1[0] += arr1[0] += 1;
+            arr2[0] += arr2[0] += 1;
+
             s1 += s1 += "dude";
             s2 += s2 += "dude";
 
-            System.out.println("s1 = " + s1);
-            System.out.println("s2 = " + s2);
+            return (i == j && i == 3 &&
+                    arr1[0] == arr2[0] && arr2[0] == 3 &&
+                    s1.equals(s2) && s1.endsWith("Hi, Hi, dude"));
+        }
+
+        public boolean test2() {
+            (i) += (i) += 1;
+            (j) += (j) += 1;
+
+            (arr1[0])+= (arr1[0]) += 1;
+            (arr2[0])+= (arr2[0]) += 1;
+
+            (s1) += (s1) += "dude";
+            (s2) += (s2) += "dude";
 
             return (i == j && i == 3 &&
+                    arr1[0] == arr2[0] && arr2[0] == 3 &&
                     s1.equals(s2) && s1.endsWith("Hi, Hi, dude"));
         }
     }

test/tools/javac/8009170/RedundantByteCodeInArrayTest.java

+/*
+ * Copyright (c) 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.
+ */
+
+/*
+ * @test
+ * @bug 8009170
+ * @summary Regression: javac generates redundant bytecode in assignop involving
+ * arrays
+ * @run main RedundantByteCodeInArrayTest
+ */
+
+import java.io.File;
+import java.io.IOException;
+
+import com.sun.tools.classfile.Attribute;
+import com.sun.tools.classfile.ClassFile;
+import com.sun.tools.classfile.Code_attribute;
+import com.sun.tools.classfile.Code_attribute.InvalidIndex;
+import com.sun.tools.classfile.ConstantPool;
+import com.sun.tools.classfile.ConstantPoolException;
+import com.sun.tools.classfile.Descriptor.InvalidDescriptor;
+import com.sun.tools.classfile.Method;
+
+public class RedundantByteCodeInArrayTest {
+    public static void main(String[] args)
+            throws IOException, ConstantPoolException, InvalidDescriptor, InvalidIndex {
+        new RedundantByteCodeInArrayTest()
+                .checkClassFile(new File(System.getProperty("test.classes", "."),
+                    RedundantByteCodeInArrayTest.class.getName() + ".class"));
+    }
+
+    void arrMethod(int[] array, int p, int inc) {
+        array[p] += inc;
+    }
+
+    void checkClassFile(File file)
+            throws IOException, ConstantPoolException, InvalidDescriptor, InvalidIndex {
+        ClassFile classFile = ClassFile.read(file);
+        ConstantPool constantPool = classFile.constant_pool;
+
+        //lets get all the methods in the class file.
+        for (Method method : classFile.methods) {
+            if (method.getName(constantPool).equals("arrMethod")) {
+                Code_attribute code = (Code_attribute) method.attributes
+                        .get(Attribute.Code);
+                if (code.max_locals > 4)
+                    throw new AssertionError("Too many locals for method arrMethod");
+            }
+        }
+    }
+}