[PATCH] 8147527: Non-optimal code generated for postfix unary operators
bsrbnd
bsrbnd at gmail.com
Mon Nov 7 12:59:02 UTC 2016
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