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

[bsrbnd]

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.

> 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));