[PATCH] 8147527: Non-optimal code generated for postfix unary operators
bsrbnd
bsrbnd at gmail.com
Mon Oct 24 11:34:27 UTC 2016
Hi,
This issue is definitly very strange...
Examining deeply (with javap) my last patch, I came to the conclusion
that the changeset for issue 8143388 is perhaps wrong.
Consider the following example:
class Issue8147527 extends Parent {
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.i);
System.out.println(in.testParent());
System.out.println(super.i);
}
Integer i=0;
private Integer test() {
return this.i++;
}
class Inner {
private Integer test() {
return Issue8147527.this.i++;
}
private Integer testParent() {
return Issue8147527.super.i++;
}
}
private Integer testParent() {
return super.i++;
}
}
class Parent {
protected Integer i=10;
}
Running it gives the following output:
0
1
1
2
10
11
11
11
If I rollback issue 8143388 changeset, I get the following output;
last line looks better (I think):
0
1
1
2
10
11
11
12
Examining the generated code of "Inner" (I added a comment surrounded
with !!!), we had:
private java.lang.Integer testParent();
Code:
0: aload_0
1: getfield #3 // Field this$0:LIssue8147527;
4: invokestatic #8 // Method
Issue8147527.access$201:(LIssue8147527;)Ljava/lang/Integer;
7: astore_1 // !!! REFERENCE TO VALUE
OF Field Parent.i:Ljava/lang/Integer; !!!
8: aload_0
9: getfield #3 // Field this$0:LIssue8147527;
12: astore_3
13: aload_3
14: aload_3
15: getfield #5 // Field
Issue8147527.i:Ljava/lang/Integer;
18: invokevirtual #6 // Method
java/lang/Integer.intValue:()I
21: iconst_1
22: iadd
23: invokestatic #7 // Method
java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
26: dup_x1
27: putfield #5 // Field
Issue8147527.i:Ljava/lang/Integer;
30: astore_2
31: aload_1 // !!! REFERENCE TO VALUE
OF Field Parent.i:Ljava/lang/Integer; !!!
32: areturn
And rollbacking issue 8143388 changeset, we have (which looks better, I think):
private java.lang.Integer testParent();
Code:
0: aload_0
1: getfield #3 // Field this$0:LIssue8147527;
4: astore_1
5: aload_1
6: getfield #8 // Field
Parent.i:Ljava/lang/Integer;
9: astore_2
10: aload_1
11: aload_1
12: getfield #8 // Field
Parent.i:Ljava/lang/Integer;
15: invokevirtual #6 // Method
java/lang/Integer.intValue:()I
18: iconst_1
19: iadd
20: invokestatic #7 // Method
java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
23: dup_x1
24: putfield #8 // Field
Parent.i:Ljava/lang/Integer;
27: astore_3
28: aload_2
29: areturn
Is issue 8143388 changeset really correct and harmless?
Thanks,
Bernard
2016-10-22 18:05 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
> Hi,
>
> Next is a probably better patch (derived from issue 8143388 changeset)
> that handles also "this, this$0, ..." in addition to the existing
> "super" handling. It optimizes the both cases ("this" and "super")
> described in issue 8147527.
> Notice that "thisDollar" could be part of the "Names" class if necessary.
>
> 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,8 +2218,10 @@
> return builder.build(rval);
> }
> Name name = TreeInfo.name(rval);
> - if (name == names._super)
> + Name thisDollar = names.fromString(names._this + "$");
> + if (name != null && (name == names._super || name ==
> names._this || name.startsWith(thisDollar))) {
> return builder.build(rval);
> + }
> VarSymbol var =
> new VarSymbol(FINAL|SYNTHETIC,
> names.fromString(
>
>
> 2016-10-10 13:45 GMT+02:00 bsrbnd <bsrbnd at gmail.com>:
>> Hi,
>>
>> Consider the following example slightly modified from issue 8147527
>> description to incorporate an assignment operator and an inner class
>> which are both of them involved in the optimization process:
>>
>> class Issue8147527 {
>> Integer i=0;
>> private Integer test() {
>> this.i += 3;
>> for (; i<5; this.i++);
>> return this.i++;
>> }
>>
>> Integer j=10;
>> class Inner {
>> private Integer test() {
>> return Issue8147527.this.j++;
>> }
>> }
>>
>> public static void main(String[] args) {
>> Issue8147527 self = new Issue8147527();
>> System.out.println(self.test());
>> System.out.println(self.i);
>>
>> Inner in = self.new Inner();
>> System.out.println(in.test());
>> System.out.println(self.j);
>> }
>> }
>>
>> The following patch omits "this" for the special case of a
>> select-expression used as an lvalue.
>> Thus we had before optimization:
>>
>> private java.lang.Integer test();
>> Code:
>> 0: aload_0
>> 1: astore_1
>> 2: aload_1
>> 3: aload_1
>> 4: getfield #3 // Field i:Ljava/lang/Integer;
>> 7: invokevirtual #5 // Method
>> java/lang/Integer.intValue:()I
>> 10: iconst_3
>> 11: iadd
>> 12: invokestatic #2 // Method
>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>> 15: dup_x1
>> 16: putfield #3 // Field i:Ljava/lang/Integer;
>>
>> And after optimization, we have:
>>
>> private java.lang.Integer test();
>> Code:
>> 0: aload_0
>> 1: aload_0
>> 2: getfield #3 // Field i:Ljava/lang/Integer;
>> 5: invokevirtual #5 // Method
>> java/lang/Integer.intValue:()I
>> 8: iconst_3
>> 9: iadd
>> 10: invokestatic #2 // Method
>> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
>> 13: putfield #3 // Field i:Ljava/lang/Integer;
>>
>> I've run all javac tests and it seems quite good.
>> Notice that I haven't checked the "super" case yet, waiting for any
>> feedback about the first optimization.
>>
>> Regards,
>> 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
>> @@ -2253,6 +2253,7 @@
>> case SELECT: {
>> final JCFieldAccess s = (JCFieldAccess)lval;
>> Symbol lid = TreeInfo.symbol(s.selected);
>> + if (lid != null && lid.name.equals(names._this)) return
>> builder.build(make.Ident(s.sym));
>> if (lid != null && lid.kind == TYP) return builder.build(lval);
>> return abstractRval(s.selected, new TreeBuilder() {
>> public JCExpression build(final JCExpression selected) {
More information about the compiler-dev
mailing list