JDK 8147527 bis: Lower.makeComma() optimization and refactoring
bsrbnd
bsrbnd at gmail.com
Tue Nov 22 12:57:48 UTC 2016
Hi,
I rewrote this optimization a bit better, here under. Creating a new
class to hold a "comma expression" is useless. I simply refactored
"LetExpr" to use statements instead of variable declarations.
I've also run all javac tests.
Bernard
2016-11-19 16:28 GMT+01:00 bsrbnd <bsrbnd at gmail.com>:
> Hi,
>
> I would like to make one more comment aside this issue but related to it.
> Lower.makeComma() creates a temporary variable and discard it
> immediatly without using it.
> This should be avoided, too. The "let expression" for the simple case
> of "this.i++" should use only one temporary variable:
>
> class Issue8147527 {
> Integer i=0;
> private Integer test() {
> return this.i++;
> }
> }
>
> The patch, here below, is a suggestion of refactoring for this
> optimization (needs to be well tested).
>
> Thus, before this we had the following desugared and generated code:
>
> private Integer test() {
> return (let /*synthetic*/ final Integer $5222779 =
> (Integer)this.i in (let /*synthetic*/ final Integer $18695331 = this.i
> = Integer.valueOf((int)(this.i.intValue() + 1)) in $5222779));
> }
>
> private java.lang.Integer test();
> Code:
> 0: aload_0
> 1: getfield #3 // Field i:Ljava/lang/Integer;
> 4: astore_1
> 5: aload_0
> 6: aload_0
> 7: getfield #3 // Field i:Ljava/lang/Integer;
> 10: invokevirtual #4 // Method
> java/lang/Integer.intValue:()I
> 13: iconst_1
> 14: iadd
> 15: invokestatic #2 // Method
> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
> 18: dup_x1
> 19: putfield #3 // Field i:Ljava/lang/Integer;
> 22: astore_2
> 23: aload_1
> 24: areturn
> }
>
> And after, we would have:
>
> private Integer test() {
> return (let /*synthetic*/ final Integer $5222779 =
> (Integer)this.i in (eval this.i =
> Integer.valueOf((int)(this.i.intValue() + 1)); in $5222779));
> }
>
> private java.lang.Integer test();
> Code:
> 0: aload_0
> 1: getfield #3 // Field i:Ljava/lang/Integer;
> 4: astore_1
> 5: aload_0
> 6: aload_0
> 7: getfield #3 // Field i:Ljava/lang/Integer;
> 10: invokevirtual #4 // Method
> java/lang/Integer.intValue:()I
> 13: iconst_1
> 14: iadd
> 15: invokestatic #2 // Method
> java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
> 18: putfield #3 // Field i:Ljava/lang/Integer;
> 21: aload_1
> 22: areturn
> }
>
> Which is much better, I think; we save space in the local variable
> array and reduce the number of instructions...
>
> What's your opinion on that, should something like this be included in
> a near future?
>
> Thanks,
>
> 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,
@@ -2283,11 +2283,9 @@
// evaluate and discard the first expression, then evaluate the second.
JCExpression makeComma(final JCExpression expr1, final
JCExpression expr2) {
- return abstractRval(expr1, new TreeBuilder() {
- public JCExpression build(final JCExpression discarded) {
- return expr2;
- }
- });
+ JCExpression comma = make.LetExpr(make.Exec(expr1), expr2);
+ comma.type = expr2.type;
+ return comma;
}
/**************************************************************************
@@ -3205,7 +3203,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 +3286,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));
@@ -3856,7 +3855,7 @@
}
public void visitLetExpr(LetExpr tree) {
- tree.defs = translateVarDefs(tree.defs);
+ tree.stmts = translate(tree.stmts);
tree.expr = translate(tree.expr, tree.type);
result = tree;
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/CRTable.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/CRTable.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/CRTable.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/CRTable.java
@@ -506,7 +506,7 @@
@Override
public void visitLetExpr(LetExpr tree) {
SourceRange sr = new SourceRange(startPos(tree), endPos(tree));
- sr.mergeWith(csp(tree.defs));
+ sr.mergeWith(csp(tree.stmts));
sr.mergeWith(csp(tree.expr));
result = sr;
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java
@@ -1574,9 +1574,9 @@
((JCUnary) e).setTag(PREDEC);
break;
}
- Assert.check(code.state.stacksize == 0);
+ Assert.check(letExprDepth != 0 || code.state.stacksize == 0);
genExpr(tree.expr, tree.expr.type).drop();
- Assert.check(code.state.stacksize == 0);
+ Assert.check(letExprDepth != 0 || code.state.stacksize == 0);
}
public void visitBreak(JCBreak tree) {
@@ -2128,7 +2128,7 @@
public void visitLetExpr(LetExpr tree) {
letExprDepth++;
int limit = code.nextreg;
- genStats(tree.defs, env);
+ genStats(tree.stmts, env);
result = genExpr(tree.expr, tree.expr.type).load();
code.endScopes(limit);
letExprDepth--;
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java
@@ -2837,10 +2837,10 @@
/** (let int x = 3; in x+2) */
public static class LetExpr extends JCExpression {
- public List<JCVariableDecl> defs;
+ public List<? extends JCStatement> stmts;
public JCExpression expr;
- protected LetExpr(List<JCVariableDecl> defs, JCExpression expr) {
- this.defs = defs;
+ protected LetExpr(List<? extends JCStatement> stmts,
JCExpression expr) {
+ this.stmts = stmts;
this.expr = expr;
}
@Override
@@ -2951,7 +2951,7 @@
JCProvides Provides(JCExpression serviceName, JCExpression implName);
JCRequires Requires(boolean isPublic, JCExpression qualId);
JCUses Uses(JCExpression qualId);
- LetExpr LetExpr(List<JCVariableDecl> defs, JCExpression expr);
+ LetExpr LetExpr(List<? extends JCStatement> stmts, JCExpression expr);
}
/** A generic visitor class for trees.
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/Pretty.java
@@ -1433,7 +1433,7 @@
public void visitLetExpr(LetExpr tree) {
try {
- print("(let " + tree.defs + " in " + tree.expr + ")");
+ print("(let " + tree.stmts + " in " + tree.expr + ")");
} catch (IOException e) {
throw new UncheckedIOException(e);
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCopier.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCopier.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCopier.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCopier.java
@@ -549,9 +549,9 @@
switch (tree.getTag()) {
case LETEXPR: {
LetExpr t = (LetExpr) node;
- List<JCVariableDecl> defs = copy(t.defs, p);
+ List<? extends JCStatement> stmts = copy(t.stmts, p);
JCExpression expr = copy(t.expr, p);
- return M.at(t.pos).LetExpr(defs, expr);
+ return M.at(t.pos).LetExpr(stmts, expr);
}
default:
throw new AssertionError("unknown tree tag: " + tree.getTag());
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeMaker.java
@@ -588,8 +588,8 @@
return tree;
}
- public LetExpr LetExpr(List<JCVariableDecl> defs, JCExpression expr) {
- LetExpr tree = new LetExpr(defs, expr);
+ public LetExpr LetExpr(List<? extends JCStatement> stmts,
JCExpression expr) {
+ LetExpr tree = new LetExpr(stmts, expr);
tree.pos = pos;
return tree;
}
@@ -609,8 +609,8 @@
defs);
}
- public LetExpr LetExpr(JCVariableDecl def, JCExpression expr) {
- LetExpr tree = new LetExpr(List.of(def), expr);
+ public LetExpr LetExpr(JCStatement stmt, JCExpression expr) {
+ LetExpr tree = new LetExpr(List.of(stmt), expr);
tree.pos = pos;
return tree;
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeScanner.java
@@ -360,7 +360,7 @@
}
public void visitLetExpr(LetExpr tree) {
- scan(tree.defs);
+ scan(tree.stmts);
scan(tree.expr);
}
diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
@@ -419,7 +419,7 @@
}
public void visitLetExpr(LetExpr tree) {
- tree.defs = translateVarDefs(tree.defs);
+ tree.stmts = translate(tree.stmts);
tree.expr = translate(tree.expr);
result = tree;
}
More information about the compiler-dev
mailing list