[PATCH] 8147527: Non-optimal code generated for postfix unary operators
bsrbnd
bsrbnd at gmail.com
Mon Nov 7 21:48:08 UTC 2016
Hi,
Another solution would be to clone the nodes as the unary boxed
operation is expanded.
This way, there is no reused tree at all and Lower.visitSelect() has
no need to be modified.
It's probably the easiest solution which is also the one that makes
most sense (I think).
Below is a fix including the optimization.
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);
@@ -3292,7 +3292,7 @@
? make.TypeCast(tree.arg.type, tmp1)
: tmp1;
JCExpression update = makeAssignop(opcode,
- lhs,
+
(JCExpression)lhs.clone(),
make.Literal(1));
return makeComma(update, tmp2);
}
2016-11-07 13:59 GMT+01:00 bsrbnd <bsrbnd at gmail.com>:
> Hi,
>
> 2016-11-06 14:38 GMT+01:00 bsrbnd <bsrbnd at gmail.com>:
>> Hi Jan,
>>
>> 2016-11-03 14:45 GMT+01:00 Jan Lahoda <jan.lahoda at oracle.com>:
>>> Hi Bernard,
>>>
>>> Thanks for looking at this. For the avoidance of the modification of
>>> tree.selected in Lower.visitSelect, what I meant was more that we would
>>> create a new instance of JCFieldAccess, to carry the new select. Might be
>>> easier that trying to work with the existing select.
>>>
>>> I think it might be useful to have a set of tests covering the usecases, so
>>> that we can more easily test patches (we will need tests eventually anyway).
>>> I can look at that, unless you want to.
>>>
>>> On 3.11.2016 13:28, bsrbnd wrote:
>>>>
>>>> Here is a short summary of this issue.
>>>>
>>>> 1) The problem related to issue 8143388 changeset (which is probably a
>>>> new issue) discovered here:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-October/010452.html
>>>>
>>>> is solved by the fix proposed here:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-October/010487.html
>>>>
>>>> 2) The optimization expected in issue 8147527 is obtained with the
>>>> following patch:
>>>>
>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-October/010448.html
>>>>
>>>> 3) I've added two additional checks (for the optimization) that verify
>>>> if this$n:
>>>> a) is synthetic
>>>> b) matches the exact regex "this\$\d+" (instead of startsWith("this$")).
>>>
>>>
>>> Offhand, this seems a little bit scary.
>>>
>>> Jan
>>>
>>>
>> Next is a safer patch (including the optimization) which ensures that
>> Lower.visitSelect():
>> 1) creates a new node, even if based on the original tree, by cloning it;
>> 2) doesn't modify the original tree (Issue8147527.super.i) by
>> restoring "tree.selected".
>>
>> Notice that it would perhaps be better to clone the tree at the start
>> of Lower.visitSelect(), letting the original node unmodified. But I
>> encountered a problem that I haven't solved yet.
>>
> The problem is that the enclosing operation refers to the original
> node which fails Lower.accessCode(). The following patch is a possible
> solution. I don't know if this is better than the previous one:
>
> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-November/010504.html
>
> Both of them correct the problem (all javac tests have been run for
> the first one).
> What do you think of these solutions?
>
> 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
> @@ -921,7 +921,7 @@
> * package?
> * @param refSuper Is access via a (qualified) C.super?
> */
> - MethodSymbol accessSymbol(Symbol sym, JCTree tree, JCTree enclOp,
> + MethodSymbol accessSymbol(Symbol sym, JCTree tree, JCTree
> original, JCTree enclOp,
> boolean protAccess, boolean refSuper) {
> ClassSymbol accOwner = refSuper && protAccess
> // For access via qualified super (T.super.x), place the
> @@ -954,7 +954,7 @@
> List<Type> thrown; // The thrown exceptions of the
> access method.
> switch (vsym.kind) {
> case VAR:
> - acode = accessCode(tree, enclOp);
> + acode = accessCode(original, enclOp);
> if (acode >= AccessCode.FIRSTASGOP.code) {
> OperatorSymbol operator = binaryAccessOperator(acode,
> enclOp.getTag());
> if (operator.opcode == string_add)
> @@ -1085,6 +1085,9 @@
> * @param refSuper Is access via a (qualified) C.super?
> */
> JCExpression access(Symbol sym, JCExpression tree, JCExpression
> enclOp, boolean refSuper) {
> + return access(sym, tree, tree, enclOp, refSuper);
> + }
> + JCExpression access(Symbol sym, JCExpression tree, JCExpression
> original, JCExpression enclOp, boolean refSuper) {
> // Access a free variable via its proxy, or its proxy's proxy
> while (sym.kind == VAR && sym.owner.kind == MTH &&
> sym.owner.enclClass() != currentClass) {
> @@ -1167,7 +1170,7 @@
> args = args.prepend(base);
> base = null; // so we don't duplicate code
> }
> - Symbol access = accessSymbol(sym, tree,
> + Symbol access = accessSymbol(sym, tree, original,
> enclOp, protAccess,
> refSuper);
> JCExpression receiver = make.Select(
> @@ -2218,7 +2221,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,
> @@ -3829,7 +3832,8 @@
> result = tree;
> }
>
> - public void visitSelect(JCFieldAccess tree) {
> + public void visitSelect(JCFieldAccess fieldAccess) {
> + JCFieldAccess tree = (JCFieldAccess)fieldAccess.clone();
> // need to special case-access of the form C.super.x
> // these will always need an access method, unless C
> // is a default interface subclassed by the current class.
> @@ -3852,7 +3856,7 @@
> result = makeThis(tree.pos(), tree.selected.type.tsym);
> }
> else
> - result = access(tree.sym, tree, enclOp, qualifiedSuperAccess);
> + result = access(tree.sym, tree, fieldAccess, enclOp,
> qualifiedSuperAccess);
> }
>
> public void visitLetExpr(LetExpr tree) {
>
>> For the optimization, the handling of "this$n" in Lower.abstractRval()
>> isn't needed anymore, which looks better because "this$n" shouldn't
>> appear when building the "let expression" as it is translated after
>> lowerBoxedPostop() in Lower.visitUnary().
>>
>> 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,
>> @@ -3837,6 +3837,7 @@
>> tree.selected.hasTag(SELECT) &&
>> TreeInfo.name(tree.selected) == names._super &&
>> !types.isDirectSuperInterface(((JCFieldAccess)tree.selected).selected.type.tsym,
>> currentClass);
>> + JCExpression original = tree.selected;
>> tree.selected = translate(tree.selected);
>> if (tree.name == names._class) {
>> result = classOf(tree.selected);
>> @@ -3853,6 +3854,9 @@
>> }
>> else
>> result = access(tree.sym, tree, enclOp, qualifiedSuperAccess);
>> +
>> + result = result == tree ? (JCExpression)tree.clone() : result;
>> + tree.selected = original;
>> }
>>
>> public void visitLetExpr(LetExpr tree) {
>>
>>>>
>>>> The following example (derived from issue 8147527 with some additional
>>>> cases) shows the result of the optimization:
>>>>
>>>> class Issue8147527 extends p.P {
>>>> public static void main(String[] args) {
>>>> Issue8147527 self = new Issue8147527();
>>>> self.testAll();
>>>> }
>>>>
>>>> private void testAll() {
>>>> System.out.println(test());
>>>> System.out.println(i);
>>>>
>>>> Inner in = new Inner();
>>>> System.out.println(in.test());
>>>> System.out.println(i);
>>>>
>>>> System.out.println(testParent());
>>>> System.out.println(super.j);
>>>>
>>>> System.out.println(in.testParent());
>>>> System.out.println(super.j);
>>>> }
>>>>
>>>> Integer i=0;
>>>> private Integer test() {
>>>> i++;
>>>> return this.i++;
>>>> }
>>>> private Integer testParent() {
>>>> j++;
>>>> return super.j++;
>>>> }
>>>>
>>>> class Inner {
>>>> private Integer test() {
>>>> i++;
>>>> return Issue8147527.this.i++;
>>>> }
>>>> private Integer testParent() {
>>>> j++;
>>>> return Issue8147527.super.j++;
>>>> }
>>>> }
>>>> }
>>>>
>>>> package p;
>>>>
>>>> public class P {
>>>> protected Integer j=20;
>>>> }
>>>>
>>>> The desugared code looks like this (as expected in issue 8147527):
>>>>
>>>> class Issue8147527$Inner {
>>>>
>>>> private Integer test() {
>>>> (let /*synthetic*/ final Integer $647942 = this$0.i in (let
>>>> /*synthetic*/ final Integer $29472086 = this$0.i =
>>>> Integer.valueOf((int)(this$0.i.intValue() + 1)) in $647942));
>>>> return (let /*synthetic*/ final Integer $8278885 =
>>>> (Integer)this$0.i in (let /*synthetic*/ final Integer $16578475 =
>>>> this$0.i = Integer.valueOf((int)(this$0.i.intValue() + 1)) in
>>>> $8278885));
>>>> }
>>>>
>>>> private Integer testParent() {
>>>> (let /*synthetic*/ final Integer $798605 =
>>>> Issue8147527.access$200(this$0) in (let /*synthetic*/ final Integer
>>>> $5995540 = Issue8147527.access$302(this$0,
>>>> Integer.valueOf((int)(Issue8147527.access$400(this$0).intValue() +
>>>> 1))) in $798605));
>>>> return (let /*synthetic*/ final Integer $33017129 =
>>>> (Integer)Issue8147527.access$501(this$0) in (let /*synthetic*/ final
>>>> Integer $25625839 = Issue8147527.access$603(this$0,
>>>> Integer.valueOf((int)(Issue8147527.access$701(this$0).intValue() +
>>>> 1))) in $33017129));
>>>> }
>>>> }
>>>>
>>>> class Issue8147527 extends p.P {
>>>>
>>>> private Integer test() {
>>>> (let /*synthetic*/ final Integer $21617 = i in (let
>>>> /*synthetic*/ final Integer $20066205 = i =
>>>> Integer.valueOf((int)(i.intValue() + 1)) in $21617));
>>>> return (let /*synthetic*/ final Integer $32619253 =
>>>> (Integer)this.i in (let /*synthetic*/ final Integer $27442939 = this.i
>>>> = Integer.valueOf((int)(this.i.intValue() + 1)) in $32619253));
>>>> }
>>>>
>>>> private Integer testParent() {
>>>> (let /*synthetic*/ final Integer $4208220 = j in (let
>>>> /*synthetic*/ final Integer $32359562 = j =
>>>> Integer.valueOf((int)(j.intValue() + 1)) in $4208220));
>>>> return (let /*synthetic*/ final Integer $20049680 =
>>>> (Integer)super.j in (let /*synthetic*/ final Integer $28368043 =
>>>> super.j = Integer.valueOf((int)(super.j.intValue() + 1)) in
>>>> $20049680));
>>>> }
>>>> }
>>>>
>>>> Putting all (1, 2 and 3) together gives the patch below.
>>>>
>>>> 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
>>>> @@ -926,7 +926,7 @@
>>>> ClassSymbol accOwner = refSuper && protAccess
>>>> // For access via qualified super (T.super.x), place the
>>>> // access symbol on T.
>>>> - ? (ClassSymbol)((JCFieldAccess) tree).selected.type.tsym
>>>> + ? (ClassSymbol)((JCFieldAccess)((JCFieldAccess)
>>>> tree).selected).selected.type.tsym
>>>> // Otherwise pretend that the owner of an accessed
>>>> // protected symbol is the enclosing class of the current
>>>> // class which is a subclass of the symbol's owner.
>>>> @@ -1100,6 +1100,7 @@
>>>> tree = make.at(tree.pos).Ident(sym);
>>>> }
>>>> JCExpression base = (tree.hasTag(SELECT)) ? ((JCFieldAccess)
>>>> tree).selected : null;
>>>> + base = refSuper ? translate(base) : base;
>>>> switch (sym.kind) {
>>>> case TYP:
>>>> if (sym.owner.kind != PCK) {
>>>> @@ -2209,16 +2210,20 @@
>>>> */
>>>> JCExpression abstractRval(JCExpression rval, Type type,
>>>> TreeBuilder builder) {
>>>> rval = TreeInfo.skipParens(rval);
>>>> + boolean synthetic = false;
>>>> switch (rval.getTag()) {
>>>> case LITERAL:
>>>> return builder.build(rval);
>>>> case IDENT:
>>>> JCIdent id = (JCIdent) rval;
>>>> + synthetic = (id.sym.flags() & SYNTHETIC) != 0;
>>>> if ((id.sym.flags() & FINAL) != 0 && id.sym.owner.kind ==
>>>> MTH)
>>>> return builder.build(rval);
>>>> }
>>>> Name name = TreeInfo.name(rval);
>>>> - if (name == names._super)
>>>> + String thisDollarN = names._this + "\\" +
>>>> target.syntheticNameChar() + "\\d+";
>>>> + boolean matchesThisDollar = synthetic && name != null &&
>>>> name.toString().matches(thisDollarN);
>>>> + if (name == names._super || name == names._this ||
>>>> matchesThisDollar)
>>>> return builder.build(rval);
>>>> VarSymbol var =
>>>> new VarSymbol(FINAL|SYNTHETIC,
>>>> @@ -3837,7 +3842,7 @@
>>>> tree.selected.hasTag(SELECT) &&
>>>> TreeInfo.name(tree.selected) == names._super &&
>>>>
>>>> !types.isDirectSuperInterface(((JCFieldAccess)tree.selected).selected.type.tsym,
>>>> currentClass);
>>>> - tree.selected = translate(tree.selected);
>>>> + if (!qualifiedSuperAccess) tree.selected =
>>>> translate(tree.selected);
>>>> if (tree.name == names._class) {
>>>> result = classOf(tree.selected);
>>>> }
>>>>
>>>
More information about the compiler-dev
mailing list