Revision: 9057
Author: [email protected]
Date: Wed Oct 13 14:39:37 2010
Log: DeadCodeElimination should only run once.
Changes to DeadCodeElimination to ensure that almost all of the
optimizations performed can be done in a single
pass. I only know of one remaining case where running the visitor twice
can result in additional changes: the
optimization performed in endVisit(JNewInstance) involving ignored new
operations to empty constructor calls.
Sometimes the constructor becomes empty later in the optimization pass, and
earlier calls aren't removed. This
seems to be a very rare case, though.
http://gwt-code-reviews.appspot.com/983802/show
Review by: [email protected]
http://code.google.com/p/google-web-toolkit/source/detail?r=9057
Modified:
/trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Simplifier.java
/trunk/dev/core/test/com/google/gwt/dev/jjs/impl/DeadCodeEliminationTest.java
=======================================
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java Tue Oct
12 13:20:21 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/ast/JModVisitor.java Wed Oct
13 14:39:37 2010
@@ -29,7 +29,7 @@
* Context for traversing a mutable list.
*/
@SuppressWarnings("unchecked")
- private class ListContext<T extends JNode> extends VisitorContext {
+ private class ListContext<T extends JNode> implements Context {
int index;
final List<T> list;
boolean removed;
@@ -50,13 +50,13 @@
public void insertAfter(JNode node) {
checkRemoved();
list.add(index + 1, (T) node);
- numMods++;
+ ++numVisitorChanges;
}
public void insertBefore(JNode node) {
checkRemoved();
list.add(index++, (T) node);
- numMods++;
+ ++numVisitorChanges;
}
public boolean isLvalue() {
@@ -67,7 +67,7 @@
checkState();
list.remove(index--);
removed = true;
- numMods++;
+ ++numVisitorChanges;
}
public void replaceMe(JNode node) {
@@ -75,7 +75,7 @@
checkReplacement(list.get(index), node);
list.set(index, (T) node);
replaced = true;
- numMods++;
+ ++numVisitorChanges;
}
/**
@@ -87,7 +87,6 @@
removed = replaced = false;
list.get(index).traverse(JModVisitor.this, this);
}
- JModVisitor.this.numVisitorChanges += numMods;
return list;
} catch (Throwable e) {
throw translateException(list.get(index), e);
@@ -112,7 +111,7 @@
* Context for traversing an immutable list.
*/
@SuppressWarnings("unchecked")
- private class ListContextImmutable<T extends JNode> extends
VisitorContext {
+ private class ListContextImmutable<T extends JNode> implements Context {
int index;
List<T> list;
boolean removed;
@@ -133,13 +132,13 @@
public void insertAfter(JNode node) {
checkRemoved();
list = Lists.add(list, index + 1, (T) node);
- numMods++;
+ ++numVisitorChanges;
}
public void insertBefore(JNode node) {
checkRemoved();
list = Lists.add(list, index++, (T) node);
- numMods++;
+ ++numVisitorChanges;
}
public boolean isLvalue() {
@@ -150,7 +149,7 @@
checkState();
list = Lists.remove(list, index--);
removed = true;
- numMods++;
+ ++numVisitorChanges;
}
public void replaceMe(JNode node) {
@@ -158,7 +157,7 @@
checkReplacement(list.get(index), node);
list = Lists.set(list, index, (T) node);
replaced = true;
- numMods++;
+ ++numVisitorChanges;
}
/**
@@ -170,7 +169,6 @@
removed = replaced = false;
list.get(index).traverse(JModVisitor.this, this);
}
- numVisitorChanges += numMods;
return list;
} catch (Throwable e) {
throw translateException(list.get(index), e);
@@ -202,7 +200,7 @@
}
}
- private static class NodeContext extends VisitorContext {
+ private class NodeContext implements Context {
boolean canRemove;
JNode node;
boolean replaced;
@@ -237,7 +235,7 @@
}
this.node = null;
- numMods++;
+ ++numVisitorChanges;
}
public void replaceMe(JNode node) {
@@ -247,17 +245,9 @@
checkReplacement(this.node, node);
this.node = node;
replaced = true;
- numMods++;
+ ++numVisitorChanges;
}
}
-
- private abstract static class VisitorContext implements Context {
- int numMods = 0;
-
- public int getNumMods() {
- return numMods;
- }
- }
protected static void checkReplacement(JNode origNode, JNode newNode) {
if (newNode == null) {
@@ -282,7 +272,6 @@
try {
ctx.node = node;
traverse(node, ctx);
- numVisitorChanges += ctx.getNumMods();
return ctx.node;
} catch (Throwable e) {
throw translateException(node, e);
@@ -302,7 +291,6 @@
ctx.replaced = false;
}
}
- numVisitorChanges += ctx.getNumMods();
} catch (Throwable e) {
throw translateException(ctx.node, e);
}
@@ -321,7 +309,6 @@
ctx.replaced = false;
}
}
- numVisitorChanges += ctx.getNumMods();
return list;
} catch (Throwable e) {
throw translateException(ctx.node, e);
@@ -334,7 +321,6 @@
try {
ctx.node = expr;
traverse(expr, ctx);
- numVisitorChanges += ctx.getNumMods();
return (JExpression) ctx.node;
} catch (Throwable e) {
throw translateException(expr, e);
@@ -369,15 +355,7 @@
* Used to determine when the optimization pass can exit.
*/
protected void madeChanges() {
- numVisitorChanges++;
- }
-
- /**
- * Call this method to indicate that a visitor has made multiple changes
to
- * the tree. Used to determine when the optimization pass can exit.
- */
- protected void madeChanges(int numMods) {
- numVisitorChanges += numMods;
+ ++numVisitorChanges;
}
protected void traverse(JNode node, Context context) {
=======================================
---
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
Tue Oct 12 13:20:21 2010
+++
/trunk/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
Wed Oct 13 14:39:37 2010
@@ -77,17 +77,28 @@
import java.util.Set;
/**
- * Attempts to remove dead code.
+ * Removes certain kinds of dead code, and simplifies certain expressions.
This
+ * pass focuses on intraprocedural optimizations, that is, optimizations
that
+ * can be performed inside of a single method body (and usually a single
+ * expression) rather than global transformations. Global optimizations
done in
+ * other passes will feed into this, however.
*/
public class DeadCodeElimination {
/**
* Eliminates dead or unreachable code when possible, and makes local
* simplifications like changing "<code>x || true</code>"
to "<code>x</code>".
*
+ * This visitor should perform all of its optimizations in a single pass.
+ * Except in rare cases, running this pass multiple times should produce
no
+ * changes after the first pass. The only currently known exception to
this
+ * rule is in {...@link #endVisit(JNewInstance, Context)}, where the target
+ * constructor may be non-empty at the beginning of DCE and become empty
+ * during the run, which potentially unlocks optimizations at call sites.
+ *
* TODO: leverage ignoring expression output more to remove intermediary
* operations in favor of pure side effects.
*
- * TODO(spoon): move more simplifications into methods like
+ * TODO: move more simplifications into methods like
* {...@link #cast(JExpression, SourceInfo, JType, JExpression)
simplifyCast}, so
* that more simplifications can be made on a single pass through a tree.
*/
@@ -131,10 +142,10 @@
}
switch (op) {
case AND:
- shortCircuitAnd(lhs, rhs, ctx);
+ maybeReplaceMe(x, simplifier.shortCircuitAnd(x, null, lhs, rhs),
ctx);
break;
case OR:
- shortCircuitOr(lhs, rhs, ctx);
+ maybeReplaceMe(x, simplifier.shortCircuitOr(x, null, lhs, rhs),
ctx);
break;
case BIT_XOR:
simplifyXor(lhs, rhs, ctx);
@@ -251,20 +262,14 @@
@Override
public void endVisit(JCastOperation x, Context ctx) {
- JExpression updated = simplifier.cast(x, x.getSourceInfo(),
- x.getCastType(), x.getExpr());
- if (updated != x) {
- ctx.replaceMe(updated);
- }
+ maybeReplaceMe(x, simplifier.cast(x, x.getSourceInfo(),
x.getCastType(),
+ x.getExpr()), ctx);
}
@Override
public void endVisit(JConditional x, Context ctx) {
- JExpression updated = simplifier.conditional(x, x.getSourceInfo(),
- x.getType(), x.getIfTest(), x.getThenExpr(), x.getElseExpr());
- if (updated != x) {
- ctx.replaceMe(updated);
- }
+ maybeReplaceMe(x, simplifier.conditional(x, x.getSourceInfo(),
+ x.getType(), x.getIfTest(), x.getThenExpr(), x.getElseExpr()),
ctx);
}
@Override
@@ -286,8 +291,7 @@
if (!booleanLiteral.getValue()) {
if (Simplifier.isEmpty(x.getBody())) {
ctx.removeMe();
- } else {
- // Unless it contains break/continue statements
+ } else { // Unless it contains break/continue statements
FindBreakContinueStatementsVisitor visitor = new
FindBreakContinueStatementsVisitor();
visitor.accept(x.getBody());
if (!visitor.hasBreakContinueStatements()) {
@@ -335,7 +339,7 @@
multi.exprs.add(literal);
}
- ctx.replaceMe(accept(multi));
+ ctx.replaceMe(this.accept(multi));
}
/**
@@ -351,7 +355,7 @@
if (!booleanLiteral.getValue()) {
JBlock block = new JBlock(x.getSourceInfo());
block.addStmts(x.getInitializers());
- ctx.replaceMe(block);
+ replaceMe(block, ctx);
}
}
}
@@ -361,11 +365,8 @@
*/
@Override
public void endVisit(JIfStatement x, Context ctx) {
- JStatement updated = simplifier.ifStatement(x, x.getSourceInfo(),
- x.getIfExpr(), x.getThenStmt(), x.getElseStmt(), currentMethod);
- if (updated != x) {
- ctx.replaceMe(updated);
- }
+ maybeReplaceMe(x, simplifier.ifStatement(x, x.getSourceInfo(),
+ x.getIfExpr(), x.getThenStmt(), x.getElseStmt(), currentMethod),
ctx);
}
@Override
@@ -425,10 +426,16 @@
@Override
public void endVisit(JMultiExpression x, Context ctx) {
List<JExpression> exprs = x.exprs;
- if (exprs.size() > 1) {
- // Remove the non-final children we previously added.
- List<JExpression> nonFinalChildren = exprs.subList(0, exprs.size()
- 1);
- ignoringExpressionOutput.removeAll(nonFinalChildren);
+ if (exprs.size() > 0) {
+ if (ignoringExpressionOutput.contains(x)) {
+ // Remove all my children we previously added.
+ ignoringExpressionOutput.removeAll(exprs);
+ } else {
+ // Remove the non-final children we previously added.
+ List<JExpression> nonFinalChildren = exprs.subList(0,
+ exprs.size() - 1);
+ ignoringExpressionOutput.removeAll(nonFinalChildren);
+ }
}
for (int i = 0; i < numRemovableExpressions(x); ++i) {
@@ -451,25 +458,31 @@
}
if (x.exprs.size() == 1) {
- ctx.replaceMe(x.exprs.get(0));
+ maybeReplaceMe(x, x.exprs.get(0), ctx);
}
}
@Override
public void endVisit(JNewInstance x, Context ctx) {
super.endVisit(x, ctx);
- if (!ignoringExpressionOutput.contains(x) |
| !x.getTarget().isEmpty()) {
- return;
- }
- // Replace the new operation with a multi.
- JMultiExpression multi = new JMultiExpression(x.getSourceInfo());
- multi.exprs.addAll(x.getArgs());
- if (x.hasClinit()) {
- multi.exprs.add(createClinitCall(x.getSourceInfo(),
- x.getTarget().getEnclosingType()));
- }
-
- ctx.replaceMe(accept(multi));
+ /*
+ * If the result of a new operation is ignored, we can remove it,
provided
+ * / it has no side effects.
+ */
+ if (ignoringExpressionOutput.contains(x)) {
+ if (!x.getTarget().isEmpty()) {
+ return;
+ }
+ JMultiExpression multi = new JMultiExpression(x.getSourceInfo());
+ multi.exprs.addAll(x.getArgs());
+ if (x.hasClinit()) {
+ multi.exprs.add(createClinitCall(x.getSourceInfo(),
+ x.getTarget().getEnclosingType()));
+ }
+ ignoringExpressionOutput.add(multi);
+ ctx.replaceMe(this.accept(multi));
+ ignoringExpressionOutput.remove(multi);
+ }
}
@Override
@@ -510,16 +523,10 @@
}
}
if (x.getOp() == JUnaryOperator.NOT) {
- JExpression updated = simplifier.not(x, x.getSourceInfo(),
x.getArg());
- if (updated != x) {
- ctx.replaceMe(updated);
- }
+ maybeReplaceMe(x, simplifier.not(x, x.getSourceInfo(),
x.getArg()), ctx);
return;
} else if (x.getOp() == JUnaryOperator.NEG) {
- JExpression updated = simplifyNegate(x, x.getArg());
- if (updated != x) {
- ctx.replaceMe(updated);
- }
+ maybeReplaceMe(x, simplifyNegate(x, x.getArg()), ctx);
}
}
@@ -573,13 +580,13 @@
removeMe(x, ctx);
} else {
// replace the try statement with just the contents of the
finally
- ctx.replaceMe(x.getFinallyBlock());
+ replaceMe(x.getFinallyBlock(), ctx);
}
} else if (noCatch && noFinally) {
// 3) Hoist up try statements with no catches and an empty finally.
// If there's no catch or finally, there's no point in this even
being
// a try statement, replace myself with the try block
- ctx.replaceMe(x.getTryBlock());
+ replaceMe(x.getTryBlock(), ctx);
}
}
@@ -648,8 +655,15 @@
public boolean visit(JMultiExpression x, Context ctx) {
List<JExpression> exprs = x.exprs;
if (exprs.size() > 0) {
- List<JExpression> nonFinalChildren = exprs.subList(0, exprs.size()
- 1);
- ignoringExpressionOutput.addAll(nonFinalChildren);
+ if (ignoringExpressionOutput.contains(x)) {
+ // None of my children matter.
+ ignoringExpressionOutput.addAll(exprs);
+ } else {
+ // Only my final child matters.
+ List<JExpression> nonFinalChildren = exprs.subList(0,
+ exprs.size() - 1);
+ ignoringExpressionOutput.addAll(nonFinalChildren);
+ }
}
return true;
}
@@ -1210,6 +1224,21 @@
private Class<?> mapType(JType type) {
return typeClassMap.get(type);
}
+
+ /**
+ * Replace me only if the the updated expression is different.
+ */
+ private void maybeReplaceMe(JExpression x, JExpression updated,
Context ctx) {
+ if (updated != x) {
+ ctx.replaceMe(updated);
+ }
+ }
+
+ private void maybeReplaceMe(JStatement x, JStatement updated, Context
ctx) {
+ if (updated != x) {
+ replaceMe(updated, ctx);
+ }
+ }
private int numRemovableExpressions(JMultiExpression x) {
if (ignoringExpressionOutput.contains(x)) {
@@ -1279,6 +1308,10 @@
}
}
+ /**
+ * Call this instead of directly calling <code>ctx.removeMe()</code>
for
+ * Statements.
+ */
private void removeMe(JStatement stmt, Context ctx) {
if (ctx.canRemove()) {
ctx.removeMe();
@@ -1289,64 +1322,18 @@
}
/**
- * Simplify short circuit AND expressions.
*
- * <pre>
- * if (true && isWhatever()) -> if (isWhatever())
- * if (false && isWhatever()) -> if (false)
- *
- * if (isWhatever() && true) -> if (isWhatever())
- * if (isWhatever() && false) -> if (false), unless side effects
- * </pre>
+ * Call this instead of directly calling <code>ctx.replaceMe()</code>
for
+ * Statements.
*/
- private void shortCircuitAnd(JExpression lhs, JExpression rhs, Context
ctx) {
- if (lhs instanceof JBooleanLiteral) {
- JBooleanLiteral booleanLiteral = (JBooleanLiteral) lhs;
- if (booleanLiteral.getValue()) {
- ctx.replaceMe(rhs);
- } else {
- ctx.replaceMe(lhs);
- }
-
- } else if (rhs instanceof JBooleanLiteral) {
- JBooleanLiteral booleanLiteral = (JBooleanLiteral) rhs;
- if (booleanLiteral.getValue()) {
- ctx.replaceMe(lhs);
- } else if (!lhs.hasSideEffects()) {
- ctx.replaceMe(rhs);
- }
+ private void replaceMe(JStatement stmt, Context ctx) {
+ stmt = this.accept(stmt, ctx.canRemove());
+ if (stmt == null) {
+ ctx.removeMe();
+ } else {
+ ctx.replaceMe(stmt);
}
}
-
- /**
- * Simplify short circuit OR expressions.
- *
- * <pre>
- * if (true || isWhatever()) -> if (true)
- * if (false || isWhatever()) -> if (isWhatever())
- *
- * if (isWhatever() || false) -> if (isWhatever())
- * if (isWhatever() || true) -> if (true), unless side effects
- * </pre>
- */
- private void shortCircuitOr(JExpression lhs, JExpression rhs, Context
ctx) {
- if (lhs instanceof JBooleanLiteral) {
- JBooleanLiteral booleanLiteral = (JBooleanLiteral) lhs;
- if (booleanLiteral.getValue()) {
- ctx.replaceMe(lhs);
- } else {
- ctx.replaceMe(rhs);
- }
-
- } else if (rhs instanceof JBooleanLiteral) {
- JBooleanLiteral booleanLiteral = (JBooleanLiteral) rhs;
- if (!booleanLiteral.getValue()) {
- ctx.replaceMe(lhs);
- } else if (!lhs.hasSideEffects()) {
- ctx.replaceMe(rhs);
- }
- }
- }
private boolean simplifyAdd(JExpression lhs, JExpression rhs, Context
ctx,
JType type) {
@@ -1666,7 +1653,7 @@
JBlock body = x.getBody();
if (body.getStatements().size() == 0) {
// Empty switch; just run the switch condition.
- ctx.replaceMe(x.getExpr().makeStatement());
+ replaceMe(x.getExpr().makeStatement(), ctx);
} else if (body.getStatements().size() == 2) {
/*
* If there are only two statements, we know it's a case statement
and
@@ -1707,13 +1694,13 @@
block.addStmt(statement);
JIfStatement ifStatement = new JIfStatement(x.getSourceInfo(),
compareOperation, block, null);
- ctx.replaceMe(ifStatement);
+ replaceMe(ifStatement, ctx);
} else {
// All we have is a default case; convert to a JBlock.
JBlock block = new JBlock(x.getSourceInfo());
block.addStmt(x.getExpr().makeStatement());
block.addStmt(statement);
- ctx.replaceMe(block);
+ replaceMe(block, ctx);
}
}
}
@@ -1813,17 +1800,9 @@
Event optimizeEvent =
SpeedTracerLogger.start(CompilerEventType.OPTIMIZE,
"optimizer", NAME);
- // TODO(zundel): see if this loop can be removed and all work done in
one
- // pass of the optimizer to improve performance.
- while (true) {
- DeadCodeVisitor deadCodeVisitor = new DeadCodeVisitor();
- deadCodeVisitor.accept(node);
- stats.recordModified(deadCodeVisitor.getNumMods());
- if (!deadCodeVisitor.didChange()) {
- break;
- }
- }
-
+ DeadCodeVisitor deadCodeVisitor = new DeadCodeVisitor();
+ deadCodeVisitor.accept(node);
+ stats.recordModified(deadCodeVisitor.getNumMods());
optimizeEvent.end("didChange", "" + stats.didChange());
return stats;
}
=======================================
--- /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Simplifier.java Tue
Mar 9 10:54:56 2010
+++ /trunk/dev/core/src/com/google/gwt/dev/jjs/impl/Simplifier.java Wed Oct
13 14:39:37 2010
@@ -26,6 +26,7 @@
import com.google.gwt.dev.jjs.ast.JExpressionStatement;
import com.google.gwt.dev.jjs.ast.JIfStatement;
import com.google.gwt.dev.jjs.ast.JMethod;
+import com.google.gwt.dev.jjs.ast.JNode;
import com.google.gwt.dev.jjs.ast.JPrefixOperation;
import com.google.gwt.dev.jjs.ast.JPrimitiveType;
import com.google.gwt.dev.jjs.ast.JProgram;
@@ -84,8 +85,9 @@
this.program = program;
}
- public JExpression cast(JExpression original, SourceInfo sourceInfo,
- JType type, JExpression exp) {
+ public JExpression cast(JExpression original, SourceInfo info, JType
type,
+ JExpression exp) {
+ info = getBestSourceInfo(original, info, exp);
if (type == exp.getType()) {
return exp;
}
@@ -116,24 +118,25 @@
if (original != null) {
return original;
}
- return new JCastOperation(sourceInfo, type, exp);
+ return new JCastOperation(info, type, exp);
}
public JExpression cast(JType type, JExpression exp) {
return cast(null, exp.getSourceInfo(), type, exp);
}
- public JExpression conditional(JConditional original, SourceInfo
sourceInfo,
+ public JExpression conditional(JConditional original, SourceInfo info,
JType type, JExpression condExpr, JExpression thenExpr,
JExpression elseExpr) {
+ info = getBestSourceInfo(original, info, condExpr);
if (condExpr instanceof JMultiExpression) {
// (a,b,c)?d:e -> a,b,(c?d:e)
// TODO(spoon): do this outward multi movement for all AST nodes
JMultiExpression condMulti = (JMultiExpression) condExpr;
- JMultiExpression newMulti = new JMultiExpression(sourceInfo);
+ JMultiExpression newMulti = new JMultiExpression(info);
newMulti.exprs.addAll(allButLast(condMulti.exprs));
- newMulti.exprs.add(conditional(null, sourceInfo, type,
- last(condMulti.exprs), thenExpr, elseExpr));
+ newMulti.exprs.add(conditional(null, info, type,
last(condMulti.exprs),
+ thenExpr, elseExpr));
// TODO(spoon): immediately simplify the resulting multi
return newMulti;
}
@@ -148,36 +151,26 @@
} else if (thenExpr instanceof JBooleanLiteral) {
if (((JBooleanLiteral) thenExpr).getValue()) {
// e.g. (cond ? true : else) -> cond || else
- JBinaryOperation binOp = new JBinaryOperation(sourceInfo, type,
- JBinaryOperator.OR, condExpr, elseExpr);
- return binOp;
+ return shortCircuitOr(null, info, condExpr, elseExpr);
} else {
// e.g. (cond ? false : else) -> !cond && else
- JPrefixOperation notCondExpr = new JPrefixOperation(
- condExpr.getSourceInfo(), JUnaryOperator.NOT, condExpr);
- JBinaryOperation binOp = new JBinaryOperation(sourceInfo, type,
- JBinaryOperator.AND, notCondExpr, elseExpr);
- return binOp;
+ JExpression notCondExpr = not(null, condExpr.getSourceInfo(),
condExpr);
+ return shortCircuitAnd(null, info, notCondExpr, elseExpr);
}
} else if (elseExpr instanceof JBooleanLiteral) {
if (((JBooleanLiteral) elseExpr).getValue()) {
// e.g. (cond ? then : true) -> !cond || then
- JPrefixOperation notCondExpr = new JPrefixOperation(
- condExpr.getSourceInfo(), JUnaryOperator.NOT, condExpr);
- JBinaryOperation binOp = new JBinaryOperation(sourceInfo, type,
- JBinaryOperator.OR, notCondExpr, thenExpr);
- return binOp;
+ JExpression notCondExpr = not(null, condExpr.getSourceInfo(),
condExpr);
+ return shortCircuitOr(null, info, notCondExpr, thenExpr);
} else {
// e.g. (cond ? then : false) -> cond && then
- JBinaryOperation binOp = new JBinaryOperation(sourceInfo, type,
- JBinaryOperator.AND, condExpr, thenExpr);
- return binOp;
+ return shortCircuitAnd(null, info, condExpr, thenExpr);
}
} else {
// e.g. (!cond ? then : else) -> (cond ? else : then)
JExpression unflipped = maybeUnflipBoolean(condExpr);
if (unflipped != null) {
- return new JConditional(sourceInfo, type, unflipped, elseExpr,
thenExpr);
+ return new JConditional(info, type, unflipped, elseExpr, thenExpr);
}
}
@@ -185,21 +178,22 @@
if (original != null) {
return original;
}
- return new JConditional(sourceInfo, type, condExpr, thenExpr,
elseExpr);
+ return new JConditional(info, type, condExpr, thenExpr, elseExpr);
}
- public JStatement ifStatement(JIfStatement original, SourceInfo
sourceInfo,
+ public JStatement ifStatement(JIfStatement original, SourceInfo info,
JExpression condExpr, JStatement thenStmt, JStatement elseStmt,
JMethod currentMethod) {
+ info = getBestSourceInfo(original, info, condExpr);
if (condExpr instanceof JMultiExpression) {
// if(a,b,c) d else e -> {a; b; if(c) d else e; }
JMultiExpression condMulti = (JMultiExpression) condExpr;
- JBlock newBlock = new JBlock(sourceInfo);
+ JBlock newBlock = new JBlock(info);
for (JExpression expr : allButLast(condMulti.exprs)) {
newBlock.addStmt(expr.makeStatement());
}
- newBlock.addStmt(ifStatement(null, sourceInfo, last(condMulti.exprs),
- thenStmt, elseStmt, currentMethod));
+ newBlock.addStmt(ifStatement(null, info, last(condMulti.exprs),
thenStmt,
+ elseStmt, currentMethod));
// TODO(spoon): immediately simplify the resulting block
return newBlock;
}
@@ -231,12 +225,12 @@
// TODO: this goes away when we normalize the Java AST properly.
thenStmt = ensureBlock(thenStmt);
elseStmt = ensureBlock(elseStmt);
- return ifStatement(null, sourceInfo, unflipped, elseStmt, thenStmt,
+ return ifStatement(null, info, unflipped, elseStmt, thenStmt,
currentMethod);
}
}
- JStatement rewritenStatement = rewriteIfIntoBoolean(sourceInfo,
condExpr,
+ JStatement rewritenStatement = rewriteIfIntoBoolean(info, condExpr,
thenStmt, elseStmt, currentMethod);
if (rewritenStatement != null) {
return rewritenStatement;
@@ -246,18 +240,18 @@
if (original != null) {
return original;
}
- return new JIfStatement(condExpr.getSourceInfo(), condExpr, thenStmt,
- elseStmt);
+ return new JIfStatement(info, condExpr, thenStmt, elseStmt);
}
- public JExpression not(JPrefixOperation original, SourceInfo sourceInfo,
+ public JExpression not(JPrefixOperation original, SourceInfo info,
JExpression arg) {
+ info = getBestSourceInfo(original, info, arg);
if (arg instanceof JMultiExpression) {
// !(a,b,c) -> (a,b,!c)
JMultiExpression argMulti = (JMultiExpression) arg;
- JMultiExpression newMulti = new JMultiExpression(sourceInfo);
+ JMultiExpression newMulti = new JMultiExpression(info);
newMulti.exprs.addAll(allButLast(argMulti.exprs));
- newMulti.exprs.add(not(null, sourceInfo, last(argMulti.exprs)));
+ newMulti.exprs.add(not(null, info, last(argMulti.exprs)));
// TODO(spoon): immediately simplify the newMulti
return newMulti;
}
@@ -286,8 +280,8 @@
newOp = JBinaryOperator.GTE;
}
if (newOp != null) {
- JBinaryOperation newBinOp = new
JBinaryOperation(argOp.getSourceInfo(),
- argOp.getType(), newOp, argOp.getLhs(), argOp.getRhs());
+ JBinaryOperation newBinOp = new JBinaryOperation(info,
argOp.getType(),
+ newOp, argOp.getLhs(), argOp.getRhs());
return newBinOp;
}
} else if (arg instanceof JPrefixOperation) {
@@ -298,13 +292,92 @@
if (op == JUnaryOperator.NOT) {
return argOp.getArg();
}
+ } else if (arg instanceof JBooleanLiteral) {
+ JBooleanLiteral booleanLit = (JBooleanLiteral) arg;
+ return JBooleanLiteral.get(!booleanLit.getValue());
}
// no simplification made
if (original != null) {
return original;
}
- return new JPrefixOperation(arg.getSourceInfo(), JUnaryOperator.NOT,
arg);
+ return new JPrefixOperation(info, JUnaryOperator.NOT, arg);
+ }
+
+ /**
+ * Simplify short circuit AND expressions.
+ *
+ * <pre>
+ * if (true && isWhatever()) -> if (isWhatever())
+ * if (false && isWhatever()) -> if (false)
+ *
+ * if (isWhatever() && true) -> if (isWhatever())
+ * if (isWhatever() && false) -> if (false), unless side effects
+ * </pre>
+ */
+ public JExpression shortCircuitAnd(JBinaryOperation original,
+ SourceInfo info, JExpression lhs, JExpression rhs) {
+ info = getBestSourceInfo(original, info, lhs);
+ if (lhs instanceof JBooleanLiteral) {
+ JBooleanLiteral booleanLiteral = (JBooleanLiteral) lhs;
+ if (booleanLiteral.getValue()) {
+ return rhs;
+ } else {
+ return lhs;
+ }
+
+ } else if (rhs instanceof JBooleanLiteral) {
+ JBooleanLiteral booleanLiteral = (JBooleanLiteral) rhs;
+ if (booleanLiteral.getValue()) {
+ return lhs;
+ } else if (!lhs.hasSideEffects()) {
+ return rhs;
+ }
+ }
+ // no simplification made
+ if (original != null) {
+ return original;
+ }
+ return new JBinaryOperation(info, rhs.getType(), JBinaryOperator.AND,
lhs,
+ rhs);
+ }
+
+ /**
+ * Simplify short circuit OR expressions.
+ *
+ * <pre>
+ * if (true || isWhatever()) -> if (true)
+ * if (false || isWhatever()) -> if (isWhatever())
+ *
+ * if (isWhatever() || false) -> if (isWhatever())
+ * if (isWhatever() || true) -> if (true), unless side effects
+ * </pre>
+ */
+ public JExpression shortCircuitOr(JBinaryOperation original, SourceInfo
info,
+ JExpression lhs, JExpression rhs) {
+ info = getBestSourceInfo(original, info, lhs);
+ if (lhs instanceof JBooleanLiteral) {
+ JBooleanLiteral booleanLiteral = (JBooleanLiteral) lhs;
+ if (booleanLiteral.getValue()) {
+ return lhs;
+ } else {
+ return rhs;
+ }
+
+ } else if (rhs instanceof JBooleanLiteral) {
+ JBooleanLiteral booleanLiteral = (JBooleanLiteral) rhs;
+ if (!booleanLiteral.getValue()) {
+ return lhs;
+ } else if (!lhs.hasSideEffects()) {
+ return rhs;
+ }
+ }
+ // no simplification made
+ if (original != null) {
+ return original;
+ }
+ return new JBinaryOperation(info, rhs.getType(), JBinaryOperator.OR,
lhs,
+ rhs);
}
private JStatement ensureBlock(JStatement stmt) {
@@ -338,6 +411,18 @@
return stmt;
}
+
+ private SourceInfo getBestSourceInfo(JNode original, SourceInfo info,
+ JNode defaultNode) {
+ if (info == null) {
+ if (original == null) {
+ info = defaultNode.getSourceInfo();
+ } else {
+ info = original.getSourceInfo();
+ }
+ }
+ return info;
+ }
private JStatement rewriteIfIntoBoolean(SourceInfo sourceInfo,
JExpression condExpr, JStatement thenStmt, JStatement elseStmt,
=======================================
---
/trunk/dev/core/test/com/google/gwt/dev/jjs/impl/DeadCodeEliminationTest.java
Thu Sep 9 00:20:17 2010
+++
/trunk/dev/core/test/com/google/gwt/dev/jjs/impl/DeadCodeEliminationTest.java
Wed Oct 13 14:39:37 2010
@@ -15,7 +15,6 @@
*/
package com.google.gwt.dev.jjs.impl;
-import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.dev.jjs.ast.JMethod;
import com.google.gwt.dev.jjs.ast.JProgram;
@@ -23,6 +22,11 @@
* Tests {...@link DeadCodeElimination}.
*/
public class DeadCodeEliminationTest extends OptimizerTestBase {
+ /*
+ * TODO: this class needs more tests, and more sophisticated cases.
Especially
+ * to ensure we converge in a single pass.
+ */
+
@Override
public void setUp() throws Exception {
addSnippetClassDecl("static volatile boolean b;");
@@ -92,7 +96,7 @@
optimize("void", "if (b) {i = 1;}").intoString(
"EntryPoint.b && (EntryPoint.i = 1);");
}
-
+
public void testDoOptimization() throws Exception {
optimize("void", "do {} while (b);").intoString(
"do {",
@@ -121,6 +125,11 @@
@Override
protected boolean optimizeMethod(JProgram program, JMethod method) {
- return DeadCodeElimination.exec(program, method).didChange();
+ OptimizerStats result = DeadCodeElimination.exec(program, method);
+ if (result.didChange()) {
+ // Make sure we converge in one pass.
+ assertFalse(DeadCodeElimination.exec(program, method).didChange());
+ }
+ return result.didChange();
}
}
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
