[PATCH] 8147527: Non-optimal code generated for postfix unary operators

[bsrbnd]

Hi,

2016-11-16 11:18 GMT+01:00 bsrbnd <bsrbnd at gmail.com>:
> Hi Jan,
>
> 2016-11-15 16:59 GMT+01:00 Jan Lahoda <jan.lahoda at oracle.com>:
>> Hi Bernard,
>>
>> Sorry for delayed answer.
>>
>> I don't think there's a precedent in the current code in using
>> JCTree.clone(), but I quite like it. I incline to think the shallow clones
>> fit the needs here.
>>
>> I am attaching a patch that includes your patch, the original test broken by
>> JDK-8143388 and a test that I tried to write to verify the desugaring
>> (checks the trees after Lower). I'd like to go through the other tests to
>> see which should be added, do some more testing (in particular the code in
>> lowerBoxedPostop seems a little bit suspicious, as the newly-created type
>> cast may be cloned) and send a request for review.
> When I wrote this patch, I was more thinking of making a deep copy of
> the tree. But I realized that a shallow copy could be enough in our
> situation and would be a safer fix. In such a configuration, cloning
> only a type-cast doesn't make any sense, of course. The updated fix
> below is probably better even if this has no impact in our precise
> example.
>
The following test shows the problem if the type-cast is cloned
instead of "tmp1". Thus the updated fix I send previously seems to be
important (I think).

Bernard

diff --git a/test/tools/javac/boxing/QualBoxedPostOp2.java
b/test/tools/javac/boxing/QualBoxedPostOp2.java
new file mode 100644
--- /dev/null
+++ b/test/tools/javac/boxing/QualBoxedPostOp2.java
@@ -0,0 +1,33 @@
+/*
+ * @test
+ * @bug 8147527
+ * @summary Qualified "super" boxed unary post-operation using a type variable.
+ */
+public class QualBoxedPostOp2<T> extends Parent2<Integer> {
+    public static void main(String[] args) {
+        new QualBoxedPostOp2().testAll();
+    }
+
+    private void testAll() {
+        super.i = 10;
+
+        equals(new Inner().testParent(), 10);
+        equals(super.i, 11);
+    }
+
+    private void equals(int a, int b) {
+        if (a != b) throw new Error();
+    }
+
+    T i;
+
+    class Inner {
+        private Integer testParent() {
+            return QualBoxedPostOp2.super.i++;
+        }
+    }
+}
+
+class Parent2<T> {
+    protected T i;
+}

>> Please let me know if
>> there are some tests that should definitely be included, or if you'd have
>> any other comments.
> "BoxedPostOpOpti" and "BoxedPostOpOpti2" verify the optimization of
> the generated code by comparing it with what's generated for "i++".
> I've thought of specifying explicitly the expected generated code,
> instead; but I thought it was too precise as it might perhaps change
> in the future. Thus, your solution of comparing the desugared code is
> probably better.
>
> Another difference is that you put "Parent" in the same package while
> I put it in another one as it was the subject of issue 8143388 (but
> this isn't really important for the optimization, I think).
>
> "BoxedPostOpOpti" tests also the optimization of "C.this.i++" which
> implies the correct handling of "this$0" without generating any
> accessor.
>
> "BoxedPostOpOpti2" verifies also the optimization of "super.j++" (with
> "P" in a separate package) which was done by issue 8143388 changset.
>
> "BoxedPostOpOpti3" checks the runtime behavior of issue 8143388
> changset as the associated test verifies only the success of the
> compilation. This isn't absolutely necessary since the problem was not
> at runtime, but I thought it was safer to do so.
>
>> Thank you for your work on this,
>>      Jan
>>
> Thank you, too.
>
> Bernard
>
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java
> @@ -2218,7 +2218,7 @@
>                  return builder.build(rval);
>          }
>          Name name = TreeInfo.name(rval);
> -        if (name == names._super)
> +        if (name == names._super || name == names._this)
>              return builder.build(rval);
>          VarSymbol var =
>              new VarSymbol(FINAL|SYNTHETIC,
> @@ -3205,7 +3205,7 @@
>                                                                        newTag,
>
> tree.type,
>
> tree.rhs.type);
> -                        JCExpression expr = lhs;
> +                        JCExpression expr = (JCExpression)lhs.clone();
>                          if (expr.type != tree.type)
>                              expr = make.TypeCast(tree.type, expr);
>                          JCBinary opResult = make.Binary(newTag, expr,
> tree.rhs);
> @@ -3288,9 +3288,10 @@
>                              public JCExpression build(final
> JCExpression tmp2) {
>                                  JCTree.Tag opcode = (tree.hasTag(POSTINC))
>                                      ? PLUS_ASG : MINUS_ASG;
> -                                JCTree lhs = cast
> -                                    ? make.TypeCast(tree.arg.type, tmp1)
> -                                    : tmp1;
> +                                JCExpression lhs = (JCExpression)tmp1.clone();
> +                                lhs = cast
> +                                    ? make.TypeCast(tree.arg.type, lhs)
> +                                    : lhs;
>                                  JCExpression update = makeAssignop(opcode,
>                                                               lhs,
>                                                               make.Literal(1));