Prune if and while statements when the expression has a statically kn…

…own value but has side effects.

Preserve volatile field access when pruning.
	Change on 2015/10/30 by kstanger <kstanger@google.com>
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=106699067

translator/src/main/java/com/google/devtools/j2objc/translate/ConstantBranchPruner.java

@@ -21,10 +21,12 @@
 import com.google.devtools.j2objc.ast.BooleanLiteral;
 import com.google.devtools.j2objc.ast.DoStatement;
 import com.google.devtools.j2objc.ast.Expression;
+import com.google.devtools.j2objc.ast.ExpressionStatement;
 import com.google.devtools.j2objc.ast.IfStatement;
 import com.google.devtools.j2objc.ast.InfixExpression;
 import com.google.devtools.j2objc.ast.ParenthesizedExpression;
 import com.google.devtools.j2objc.ast.PrefixExpression;
+import com.google.devtools.j2objc.ast.TreeUtil;
 import com.google.devtools.j2objc.ast.TreeVisitor;
 import com.google.devtools.j2objc.ast.WhileStatement;
 import com.google.devtools.j2objc.util.TranslationUtil;
@@ -42,12 +44,17 @@ public class ConstantBranchPruner extends TreeVisitor {
 
   @Override
   public void endVisit(IfStatement node) {
-    Boolean value = getValue(node.getExpression());
+    Expression expr = node.getExpression();
+    Boolean value = getKnownValue(expr);
     if (value != null) {
+      Expression sideEffects = extractSideEffects(expr);
+      if (sideEffects != null) {
+        TreeUtil.insertBefore(node, new ExpressionStatement(sideEffects));
+      }
       if (value) {
-        node.replaceWith(node.getThenStatement().copy());
+        node.replaceWith(TreeUtil.remove(node.getThenStatement()));
       } else if (node.getElseStatement() != null) {
-        node.replaceWith(node.getElseStatement().copy());
+        node.replaceWith(TreeUtil.remove(node.getElseStatement()));
       } else {
         node.remove();
       }
@@ -61,25 +68,26 @@ public void endVisit(InfixExpression node) {
       return;
     }
     List<Expression> operands = node.getOperands();
-    boolean foundSideEffect = false;
-    for (Iterator<Expression> it = operands.iterator(); it.hasNext(); ) {
-      Expression expr = it.next();
-      foundSideEffect |= TranslationUtil.hasSideEffect(expr);
-      Boolean constantVal = getValue(expr);
-      if (constantVal == null) {
+    int lastSideEffect = -1;
+    for (int i = 0; i < operands.size(); i++) {
+      Expression expr = operands.get(i);
+      if (TranslationUtil.hasSideEffect(expr)) {
+        lastSideEffect = i;
+      }
+      Boolean knownVal = getKnownValue(expr);
+      if (knownVal == null) {
         continue;
       }
-      if (constantVal == (operator == CONDITIONAL_OR)) {
-        // Whole expression evaluates to 'constantVal'.
-        if (!foundSideEffect) {
-          // We can safely prune the whole infix if the precededing expressions
-          // have no side effects.
-          node.replaceWith(new BooleanLiteral(constantVal, typeEnv));
-          return;
+      if (knownVal == (operator == CONDITIONAL_OR)) {
+        // Whole expression evaluates to 'knownVal'.
+        operands.subList(lastSideEffect + 1, operands.size()).clear();
+        if (lastSideEffect < i) {
+          operands.add(new BooleanLiteral(knownVal, typeEnv));
         }
-      } else {
+        break;
+      } else if (lastSideEffect < i) {
         // Else remove unnecessary constant value.
-        it.remove();
+        operands.remove(i--);
       }
     }
 
@@ -102,7 +110,7 @@ public void endVisit(InfixExpression node) {
    */
   @Override
   public void endVisit(PrefixExpression node) {
-    Boolean value = getValue(node.getOperand());
+    Boolean value = getReplaceableValue(node.getOperand());
     if (node.getOperator() == PrefixExpression.Operator.NOT && value != null) {
       node.replaceWith(new BooleanLiteral(!value, typeEnv));
     }
@@ -113,30 +121,97 @@ public void endVisit(PrefixExpression node) {
    */
   @Override
   public void endVisit(ParenthesizedExpression node) {
-    if (getValue(node.getExpression()) != null) {
+    if (getReplaceableValue(node.getExpression()) != null) {
       node.replaceWith(node.getExpression().copy());
     }
   }
 
   @Override
   public void endVisit(WhileStatement node) {
-    if (getValue(node.getExpression()) == FALSE) {
-      node.remove();
+    Expression expr = node.getExpression();
+    if (getKnownValue(expr) == FALSE) {
+      Expression sideEffects = extractSideEffects(expr);
+      if (sideEffects != null) {
+        node.replaceWith(new ExpressionStatement(sideEffects));
+      } else {
+        node.remove();
+      }
     }
   }
 
   /**
-   * Returns TRUE or FALSE if expression is a boolean constant, else
-   * null (unknown statically).
+   * Returns TRUE or FALSE if expression is a boolean constant and has no side
+   * effects, else null (unknown statically).
+   */
+  private Boolean getReplaceableValue(Expression expr) {
+    return TranslationUtil.hasSideEffect(expr) ? null : getKnownValue(expr);
+  }
+
+  /**
+   * Returns TRUE of FALSE if 'expr' is a boolean expression and its value is
+   * known statically. The caller should be careful when replacing this
+   * expression as it may have side effects.
    */
-  private Boolean getValue(Expression expr) {
+  private Boolean getKnownValue(Expression expr) {
     Object value = expr.getConstantValue();
     if (value instanceof Boolean) {
       return (Boolean) value;
     }
-    if (expr instanceof BooleanLiteral) {
-      return ((BooleanLiteral) expr).booleanValue();
+    switch (expr.getKind()) {
+      case BOOLEAN_LITERAL:
+        return ((BooleanLiteral) expr).booleanValue();
+      case INFIX_EXPRESSION:
+        {
+          InfixExpression infixExpr = (InfixExpression) expr;
+          InfixExpression.Operator op = infixExpr.getOperator();
+          if (op == CONDITIONAL_AND || op == CONDITIONAL_OR) {
+            // We assume that this node has already been visited and pruned so
+            // if it has a known value, it will be equal to the last operand.
+            List<Expression> operands = infixExpr.getOperands();
+            Boolean lastOperand = getKnownValue(operands.get(operands.size() - 1));
+            if (lastOperand != null && lastOperand.booleanValue() == (op == CONDITIONAL_OR)) {
+              return lastOperand;
+            }
+          }
+          return null;
+        }
+      case PARENTHESIZED_EXPRESSION:
+        return getKnownValue(((ParenthesizedExpression) expr).getExpression());
+      default:
+        return null;
+    }
+  }
+
+  /**
+   * Returns an expression containing the side effects of the given expression.
+   * The evaluated result of the expression may differ from the original.
+   */
+  private Expression extractSideEffects(Expression expr) {
+    switch (expr.getKind()) {
+      case INFIX_EXPRESSION:
+        {
+          List<Expression> operands = ((InfixExpression) expr).getOperands();
+          Expression lastOperand = operands.remove(operands.size() - 1);
+          lastOperand = extractSideEffects(lastOperand);
+          if (lastOperand != null) {
+            operands.add(lastOperand);
+          }
+          if (operands.size() == 1) {
+            return operands.remove(0);
+          }
+          return TreeUtil.remove(expr);
+        }
+      case PARENTHESIZED_EXPRESSION:
+        {
+          Expression sideEffects = extractSideEffects(
+              ((ParenthesizedExpression) expr).getExpression());
+          if (sideEffects != null) {
+            return ParenthesizedExpression.parenthesize(sideEffects);
+          }
+          return null;
+        }
+      default:
+        return null;
     }
-    return null;
   }
 }

translator/src/main/java/com/google/devtools/j2objc/util/TranslationUtil.java

@@ -189,6 +189,10 @@ public static boolean isAssigned(Expression node) {
    * would be unsafe to prune the given node from the tree.
    */
   public static boolean hasSideEffect(Expression expr) {
+    IVariableBinding var = TreeUtil.getVariableBinding(expr);
+    if (var != null && BindingUtil.isVolatile(var)) {
+      return true;
+    }
     switch (expr.getKind()) {
       case BOOLEAN_LITERAL:
       case CHARACTER_LITERAL:

translator/src/test/java/com/google/devtools/j2objc/translate/ConstantBranchPrunerTest.java

@@ -151,20 +151,38 @@ public void testOr() throws IOException {
   // Verify method invocation paired with constant is not pruned because methods
   // may have side effects.
   public void testMethodAnd() throws IOException {
-    // TODO(kstanger): Find a way to prune the unreachable branches while
-    // preserving the portions of the expression that have side effects.
     String translation = translateSourceFile(
         "class Test { "
         + "static final boolean DEBUG = true; "
         + "static final boolean NDEBUG = false;"
+        + "boolean b;"
         + "boolean enabled() { return true; }"
         + "int test() { int result; "
-        + "  if (enabled() && NDEBUG) { result = 1; } else { result = 2; }"
+        // The "b" operand should be pruned since it has no side effect.
+        + "  if (enabled() && b && NDEBUG) { result = 1; } else { result = 2; }"
         + "  if (enabled() || DEBUG) { result = 3; } else { result = 4; }"
         + "  return result; }}",
         "Test", "Test.m");
-    assertTranslation(translation, "if ([self enabled] && Test_NDEBUG)");
-    assertTranslation(translation, "if ([self enabled] || Test_DEBUG)");
+    assertTranslatedLines(translation,
+        "[self enabled];",
+        "{",
+        "  result = 2;",
+        "}",
+        "[self enabled];",
+        "{",
+        "  result = 3;",
+        "}");
+  }
+
+  public void testWhileStatementPrunedWithSideEffects() throws IOException {
+    String translation = translateSourceFile(
+        "class Test { boolean getB() { return true; } int test(boolean b) { "
+        + "while (b && (getB() && false)) { return 1; } return 0; } }", "Test", "Test.m");
+    assertTranslatedLines(translation,
+        "- (jint)testWithBoolean:(jboolean)b {",
+        "  b && ([self getB]);",
+        "  return 0;",
+        "}");
   }
 
   public void testExpressionPruning() throws IOException {
@@ -181,4 +199,13 @@ public void testExpressionPruning() throws IOException {
     assertTranslatedLines(translation,
         "- (jboolean)test {", "if (A_nonConstant_) return false;", "return true;", "}");
   }
+
+  // Verify that volatile loads aren't pruned because they provide a memory
+  // barrier.
+  public void testVolatileLoad() throws IOException {
+    String translation = translateSourceFile(
+        "class Test { volatile int i; boolean test() { return i == 1 && false; } }",
+        "Test", "Test.m");
+    assertTranslation(translation, "return JreLoadVolatileInt(&i_) == 1 && false;");
+  }
 }