[PATCH] 8147527: Non-optimal code generated for postfix unary operators
Jan Lahoda
jan.lahoda at oracle.com
Tue Nov 15 15:59:09 UTC 2016
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. Please let me know if there are some tests that should
definitely be included, or if you'd have any other comments.
Thank you for your work on this,
Jan
On 15.11.2016 13:11, bsrbnd wrote:
> Hi,
>
> 2016-11-07 22:48 GMT+01:00 bsrbnd <bsrbnd at gmail.com>:
>> 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.
>>
> Of course, a deep copy of the tree would be a conceptually better
> solution but more dangerous for a simple fix.
> Unfortunately, "TreeCopier" loses some information like symbols, etc...
> For this reason, I wrote a new class "TreeCloner" extending
> "TreeCopier" which performs a full cloning of the nodes while copying
> the tree.
> I've only implemented the necessary methods required by this issue, as
> shown in the following patch (all javac tests have been run).
> What do you think of all these solutions?
>
> 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,
> @@ -3205,7 +3205,7 @@
> newTag,
>
> tree.type,
>
> tree.rhs.type);
> - JCExpression expr = lhs;
> + JCExpression expr = new TreeCloner(make).copy(lhs);
> 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,
> + new
> TreeCloner(make).copy(lhs),
> make.Literal(1));
> return makeComma(update, tmp2);
> }
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCloner.java
> b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCloner.java
> new file mode 100644
> --- /dev/null
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeCloner.java
> @@ -0,0 +1,45 @@
> +package com.sun.tools.javac.tree;
> +
> +import com.sun.source.tree.*;
> +import com.sun.tools.javac.tree.JCTree.*;
> +import com.sun.tools.javac.util.DefinedBy;
> +import com.sun.tools.javac.util.DefinedBy.Api;
> +
> +public class TreeCloner extends TreeCopier<Void> {
> + public TreeCloner(TreeMaker M) {
> + super(M);
> + }
> +
> + @DefinedBy(Api.COMPILER_TREE)
> + public JCTree visitIdentifier(IdentifierTree node, Void p) {
> + return (JCIdent)((JCIdent)node).clone();
> + }
> +
> + @DefinedBy(Api.COMPILER_TREE)
> + public JCTree visitMemberSelect(MemberSelectTree node, Void p) {
> + JCFieldAccess t = (JCFieldAccess)((JCFieldAccess)node).clone();
> + t.selected = copy(t.selected, p);
> + return t;
> + }
> +
> + @DefinedBy(Api.COMPILER_TREE)
> + public JCTree visitTypeCast(TypeCastTree node, Void p) {
> + JCTypeCast t = (JCTypeCast)((JCTypeCast)node).clone();
> + t.clazz = copy(t.clazz, p);
> + t.expr = copy(t.expr, p);
> + return t;
> + }
> +
> + @DefinedBy(Api.COMPILER_TREE)
> + public JCTree visitArrayAccess(ArrayAccessTree node, Void p) {
> + JCArrayAccess t = (JCArrayAccess)((JCArrayAccess)node).clone();
> + t.indexed = copy(t.indexed, p);
> + t.index = copy(t.index, p);
> + return t;
> + }
> +
> + @DefinedBy(Api.COMPILER_TREE)
> + public JCTree visitLiteral(LiteralTree node, Void p) {
> + return (JCLiteral)((JCLiteral)node).clone();
> + }
> +}
>
-------------- next part --------------
# HG changeset patch
# Parent 07ca23d6190c74e256be5874b9f865b456c48aba
diff -r 07ca23d6190c 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 Mon Nov 14 08:28:18 2016 +0100
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java Mon Nov 14 11:24:23 2016 +0100
@@ -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);
}
diff -r 07ca23d6190c test/tools/javac/boxing/QualBoxedPostOp.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/tools/javac/boxing/QualBoxedPostOp.java Mon Nov 14 11:24:23 2016 +0100
@@ -0,0 +1,51 @@
+/*
+ * @test
+ * @bug 8147527
+ * @summary Qualified "this" and "super" boxed unary post operations.
+ */
+public class QualBoxedPostOp extends Parent {
+ public static void main(String[] args) {
+ new QualBoxedPostOp().testAll();
+ }
+
+ private void testAll() {
+ equals(test(), 0);
+ equals(i, 1);
+
+ Inner in = new Inner();
+ equals(in.test(), 1);
+ equals(i, 2);
+
+ equals(testParent(), 10);
+ equals(super.i, 11);
+
+ equals(in.testParent(), 11);
+ equals(super.i, 12);
+ }
+
+ private void equals(int a, int b) {
+ if (a != b) throw new Error();
+ }
+
+ Integer i=0;
+
+ private Integer test() {
+ return this.i++;
+ }
+ private Integer testParent() {
+ return super.i++;
+ }
+
+ class Inner {
+ private Integer test() {
+ return QualBoxedPostOp.this.i++;
+ }
+ private Integer testParent() {
+ return QualBoxedPostOp.super.i++;
+ }
+ }
+}
+
+class Parent {
+ protected Integer i=10;
+}
diff -r 07ca23d6190c test/tools/javac/desugar/BoxingAndSuper.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/tools/javac/desugar/BoxingAndSuper.java Mon Nov 14 11:24:23 2016 +0100
@@ -0,0 +1,282 @@
+/**
+ * @test
+ * @modules jdk.compiler/com.sun.tools.javac.api
+ * jdk.compiler/com.sun.tools.javac.code
+ * jdk.compiler/com.sun.tools.javac.comp
+ * jdk.compiler/com.sun.tools.javac.tree
+ * jdk.compiler/com.sun.tools.javac.util
+ */
+
+
+import java.io.IOException;
+import java.io.StringWriter;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.TreeSet;
+
+import javax.tools.JavaCompiler;
+import javax.tools.SimpleJavaFileObject;
+import javax.tools.StandardJavaFileManager;
+import javax.tools.ToolProvider;
+
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.JavacTask;
+import com.sun.source.util.TaskEvent;
+import com.sun.source.util.TaskEvent.Kind;
+import com.sun.source.util.TaskListener;
+import com.sun.tools.javac.api.JavacTool;
+import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.comp.AttrContext;
+import com.sun.tools.javac.comp.Env;
+import com.sun.tools.javac.comp.Lower;
+import com.sun.tools.javac.tree.JCTree;
+import com.sun.tools.javac.tree.JCTree.JCBlock;
+import com.sun.tools.javac.tree.JCTree.JCExpression;
+import com.sun.tools.javac.tree.JCTree.JCFieldAccess;
+import com.sun.tools.javac.tree.JCTree.JCIdent;
+import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
+import com.sun.tools.javac.tree.JCTree.JCMethodInvocation;
+import com.sun.tools.javac.tree.JCTree.JCModifiers;
+import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
+import com.sun.tools.javac.tree.JCTree.LetExpr;
+import com.sun.tools.javac.tree.JCTree.Tag;
+import com.sun.tools.javac.tree.TreeCopier;
+import com.sun.tools.javac.tree.TreeInfo;
+import com.sun.tools.javac.tree.TreeMaker;
+import com.sun.tools.javac.tree.TreeScanner;
+import com.sun.tools.javac.util.Context;
+import com.sun.tools.javac.util.List;
+import com.sun.tools.javac.util.Log;
+import com.sun.tools.javac.util.Log.WriterKind;
+import com.sun.tools.javac.util.Names;
+
+public class BoxingAndSuper {
+ public static void main(String... args) throws Exception {
+ new BoxingAndSuper().testSuper();
+ new BoxingAndSuper().testThis();
+ }
+
+ public void testSuper() throws Exception {
+ String code = "class Test extends Parent {\n" +
+ " protected Integer i=20;\n" +
+ " class Inner {\n" +
+ " private Integer dump() {\n" +
+ " return Test.super.i++;\n" +
+ " }\n" +
+ " }\n" +
+ "}\n" +
+ "class Parent {\n" +
+ " protected Integer i=10;\n" +
+ "} ";
+ String expected =
+ "Test.Inner.dump()java.lang.Integer\n" +
+ "{\n" +
+ " return (let /*synthetic*/ final Integer $le0 = (Integer)Test.access$001(this$0) " +
+ "in (let /*synthetic*/ final Integer $le1 = Test.access$103(this$0, Integer.valueOf((int)(Test.access$201(this$0).intValue() + 1))) " +
+ "in $le0));\n" +
+ "}\n" +
+ "Test.access$001(Test)java.lang.Integer\n" +
+ "{\n" +
+ " return x0.i;\n" +
+ "}\n" +
+ "Test.access$103(Test,java.lang.Integer)java.lang.Integer\n" +
+ "{\n" +
+ " return x0.i = x1;\n" +
+ "}\n" +
+ "Test.access$201(Test)java.lang.Integer\n" +
+ "{\n" +
+ " return x0.i;\n" +
+ "}\n";
+ runTest(code, expected);
+ }
+
+ public void testThis() throws Exception {
+ String code = "public class Test {\n" +
+ " Integer i;\n" +
+ " private void dump() {\n" +
+ " i++;\n" +
+ " this.i++;\n" +
+ " }\n" +
+ "}";
+ String expected =
+ "Test.dump()void\n" +
+ "{\n" +
+ " (let /*synthetic*/ final Integer $le0 = i in (let /*synthetic*/ final Integer $le1 = i = Integer.valueOf((int)(i.intValue() + 1)) in $le0));\n" +
+ " (let /*synthetic*/ final Integer $le2 = (Integer)this.i in (let /*synthetic*/ final Integer $le3 = this.i = Integer.valueOf((int)(this.i.intValue() + 1)) in $le2));\n" +
+ "}\n";
+ runTest(code, expected);
+ }
+
+ private void runTest(String code, String expectedDesugar) throws Exception {
+ JavacTool compiler = (JavacTool) ToolProvider.getSystemJavaCompiler();
+ StringWriter out = new StringWriter();
+ Context context = new Context();
+ TestLower.preRegister(context);
+ JavacTask task = (JavacTask) compiler.getTask(out, null, null, Arrays.asList("-d", "."), null, Arrays.asList(new JFOImpl(new URI("mem:///Test.java"), code)), context);
+
+ task.generate();
+
+ out.flush();
+
+ String actual = out.toString();
+
+ if (!expectedDesugar.equals(actual)) {
+ throw new IllegalStateException("Actual does not match expected: " + actual);
+ }
+ }
+
+ private static final class TestLower extends Lower {
+
+ public static void preRegister(Context context) {
+ context.put(lowerKey, new Context.Factory<Lower>() {
+ public Lower make(Context c) {
+ return new TestLower(c);
+ }
+ });
+ }
+
+ private final TreeMaker make;
+ private final Names names;
+ private final Log log;
+
+ public TestLower(Context context) {
+ super(context);
+ make = TreeMaker.instance(context);
+ names = Names.instance(context);
+ log = Log.instance(context);
+ }
+
+ @Override
+ public List<JCTree> translateTopLevelClass(Env<AttrContext> env, JCTree cdef, TreeMaker make) {
+ List<JCTree> result = super.translateTopLevelClass(env, cdef, make);
+ Map<Symbol, JCMethodDecl> declarations = new HashMap<>();
+ Set<Symbol> toDump = new TreeSet<>(symbolComparator);
+
+ new TreeScanner() {
+ @Override
+ public void visitMethodDef(JCMethodDecl tree) {
+ if (tree.name.toString().startsWith("dump")) {
+ toDump.add(tree.sym);
+ }
+ declarations.put(tree.sym, tree);
+ super.visitMethodDef(tree);
+ }
+ }.scan(result);
+
+ for (Symbol d : toDump) {
+ dump(d, declarations, new HashSet<>());
+ }
+
+ return result;
+ }
+
+ private void dump(Symbol methodSym, Map<Symbol, JCMethodDecl> declarations, Set<Symbol> alreadyPrinted) {
+ if (!alreadyPrinted.add(methodSym))
+ return ;
+
+ JCMethodDecl method = declarations.get(methodSym);
+
+ if (method == null) {
+ return ;
+ }
+
+ log.getWriter(WriterKind.NOTICE).println(symbol2String(methodSym));
+
+ JCBlock body = new TreeCopier<Void>(make) {
+ private final Map<String, String> letExprRemap = new HashMap<>();
+ private int i;
+
+ @Override
+ public JCTree visitOther(Tree node, Void p) {
+ JCTree tree = (JCTree) node;
+ if (tree.hasTag(Tag.LETEXPR)) {
+ LetExpr le = (LetExpr) tree;
+
+ for (JCVariableDecl var : le.defs) {
+ letExprRemap.put(var.name.toString(), "$le" + i++);
+ }
+ }
+ return super.visitOther(node, p);
+ }
+
+ @Override
+ public JCTree visitVariable(VariableTree node, Void p) {
+ String newName = letExprRemap.get(node.getName().toString());
+ if (newName != null) {
+ node = make.VarDef((JCModifiers) node.getModifiers(), names.fromString(newName), (JCExpression) node.getType(), (JCExpression) node.getInitializer());
+ }
+ return super.visitVariable(node, p);
+ }
+
+ @Override
+ public JCTree visitIdentifier(IdentifierTree node, Void p) {
+ String newName = letExprRemap.get(node.getName().toString());
+ if (newName != null) {
+ node = make.Ident(names.fromString(newName));
+ }
+ return super.visitIdentifier(node, p);
+ }
+
+ @Override
+ public <T extends JCTree> T copy(T tree, Void p) {
+ if (tree.hasTag(Tag.LETEXPR)) {
+ return (T) visitOther(tree, p);
+ }
+ return super.copy(tree, p);
+ }
+
+ }.copy(method.body);
+ log.getWriter(WriterKind.NOTICE).println(body.toString());
+
+ Set<Symbol> invoked = new TreeSet<>(symbolComparator);
+
+ new TreeScanner() {
+ @Override
+ public void visitApply(JCMethodInvocation tree) {
+ invoked.add(TreeInfo.symbol(tree.meth));
+ super.visitApply(tree);
+ }
+ }.scan(method);
+
+ for (Symbol search : invoked) {
+ dump(search, declarations, alreadyPrinted);
+ }
+ }
+
+ private String symbol2String(Symbol sym) {
+ switch (sym.kind) {
+ case TYP:
+ return sym.getQualifiedName().toString();
+ case MTH:
+ return symbol2String(sym.owner) + "." + sym.name + sym.type.toString();
+ default:
+ throw new UnsupportedOperationException();
+ }
+ }
+
+ private final Comparator<Symbol> symbolComparator = (s1, s2) -> symbol2String(s1).compareTo(symbol2String(s2));
+ }
+
+ private static final class JFOImpl extends SimpleJavaFileObject {
+
+ private final String code;
+
+ public JFOImpl(URI uri, String code) {
+ super(uri, Kind.SOURCE);
+ this.code = code;
+ }
+
+ @Override
+ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException {
+ return code;
+ }
+
+ }
+}
\ No newline at end of file
More information about the compiler-dev
mailing list