Refined the reducer's facilities for unwrapping block statements. It… (…

…#402)

* Refined the reducer's facilities for unwrapping block statements.  It did not previously unwrap a statement that contained declaration statements.  Now it does, so long as doing so will not introduce name clashes.  This had some small knock-on effects on the reduction of live color writes, which can now get unwrapped so that they appear in the outer-most block of a function.

reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/LiveOutputVariableWriteReductionOpportunities.java

@@ -56,18 +56,13 @@ private LiveOutputVariableWriteReductionOpportunities(
   }
 
   @Override
-  protected void visitChildOfBlock(BlockStmt block, int index) {
-
-    final Stmt child = block.getStmt(index);
-
-    if (!(child instanceof BlockStmt)) {
-      return;
-    }
-    final Optional<String> backupName = containsOutVariableBackup((BlockStmt) child);
+  public void visitBlockStmt(BlockStmt block) {
+    super.visitBlockStmt(block);
+    final Optional<String> backupName = containsOutVariableBackup(block);
     if (backupName.isPresent()) {
-      addOpportunity(new LiveOutputVariableWriteReductionOpportunity((BlockStmt) child,
-            backupName.get(),
-            getVistitationDepth()));
+      addOpportunity(new LiveOutputVariableWriteReductionOpportunity(block,
+          backupName.get(),
+          getVistitationDepth()));
     }
   }
 

reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/UnwrapReductionOpportunities.java

@@ -17,6 +17,7 @@
 package com.graphicsfuzz.reducer.reductionopportunities;
 
 import com.graphicsfuzz.common.ast.TranslationUnit;
+import com.graphicsfuzz.common.ast.decl.VariableDeclInfo;
 import com.graphicsfuzz.common.ast.stmt.BlockStmt;
 import com.graphicsfuzz.common.ast.stmt.DeclarationStmt;
 import com.graphicsfuzz.common.ast.stmt.DoStmt;
@@ -27,7 +28,11 @@
 import com.graphicsfuzz.common.transformreduce.ShaderJob;
 import com.graphicsfuzz.common.util.ListConcat;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 public class UnwrapReductionOpportunities
       extends ReductionOpportunitiesBase<UnwrapReductionOpportunity> {
@@ -98,23 +103,33 @@ public void visitDoStmt(DoStmt doStmt) {
   }
 
   @Override
-  protected void visitChildOfBlock(BlockStmt block, int index) {
-    final Stmt child = block.getStmt(index);
-    if (isNonEmptyBlockStmtWithoutTopLevelDeclaration(child)) {
-      addOpportunity(
-            new UnwrapReductionOpportunity(child, ((BlockStmt) child).getStmts(),
-                  parentMap,
-                  getVistitationDepth()));
+  protected void visitChildOfBlock(BlockStmt block, int childIndex) {
+    final Stmt child = block.getStmt(childIndex);
+    if (!(child instanceof BlockStmt)) {
+      return;
     }
-  }
-
-  private boolean isNonEmptyBlockStmtWithoutTopLevelDeclaration(Stmt stmt) {
-    if (!(stmt instanceof BlockStmt)) {
-      return false;
+    final BlockStmt childAsBlock = (BlockStmt) child;
+    if (childAsBlock.getNumStmts() == 0) {
+      // We do not unwrap an empty block statement, as there would be nothing to unwrap.
+      // Other reductions take responsibility for removing a redundant { }.
+      return;
+    }
+    // If unwrapping the child block may lead to name collisions then it cannot be done.
+    // A collision can be with a variable name already in scope, or a variable name that will come
+    // into scope later in the parent block.
+    final Set<String> namesDeclaredDirectlyByParentOrInScopeAlready = new HashSet<>();
+    namesDeclaredDirectlyByParentOrInScopeAlready.addAll(
+        UnwrapReductionOpportunity.getNamesDeclaredDirectlyByBlock(block));
+    namesDeclaredDirectlyByParentOrInScopeAlready.addAll(currentScope.namesOfAllVariablesInScope());
+    final Set<String> namesDeclaredDirectlyByChild =
+        UnwrapReductionOpportunity.getNamesDeclaredDirectlyByBlock(childAsBlock);
+    if (Collections.disjoint(namesDeclaredDirectlyByParentOrInScopeAlready,
+        namesDeclaredDirectlyByChild)) {
+      addOpportunity(
+          new UnwrapReductionOpportunity(child, childAsBlock.getStmts(),
+              parentMap,
+              getVistitationDepth()));
     }
-    final BlockStmt blockStmt = (BlockStmt) stmt;
-    return blockStmt.getNumStmts() > 0 && blockStmt.getStmts().stream()
-          .noneMatch(item -> item instanceof DeclarationStmt);
   }
 
   private boolean isUnwrappable(LoopStmt loop) {

reducer/src/main/java/com/graphicsfuzz/reducer/reductionopportunities/UnwrapReductionOpportunity.java

@@ -17,13 +17,19 @@
 package com.graphicsfuzz.reducer.reductionopportunities;
 
 import com.graphicsfuzz.common.ast.ChildDoesNotExistException;
+import com.graphicsfuzz.common.ast.IAstNode;
 import com.graphicsfuzz.common.ast.IParentMap;
 import com.graphicsfuzz.common.ast.stmt.BlockStmt;
+import com.graphicsfuzz.common.ast.stmt.DeclarationStmt;
 import com.graphicsfuzz.common.ast.stmt.Stmt;
 import com.graphicsfuzz.common.ast.visitors.VisitationDepth;
+import com.graphicsfuzz.common.util.ListConcat;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
+import java.util.Set;
+import java.util.stream.Collectors;
 
 public final class UnwrapReductionOpportunity extends AbstractReductionOpportunity {
 
@@ -80,10 +86,38 @@ public void applyReductionImpl() {
 
   @Override
   public boolean preconditionHolds() {
-    if (!(parentMap.getParent(wrapper) instanceof Stmt)) {
+    final IAstNode parentOfWrapper = parentMap.getParent(wrapper);
+    if (!(parentOfWrapper instanceof Stmt)) {
       return false;
     }
+    if (parentOfWrapper instanceof BlockStmt) {
+      // We need to make sure it is still the case that applying the unwrap will not lead to name
+      // clashes.  We only need to check the names declared directly by the parent block against
+      // the names declared inside the block being unwrapped, because it is only these sets of names
+      // that can have been affected by applying other reduction opportunities.
+      if (!Collections.disjoint(getNamesDeclaredDirectlyByBlock((BlockStmt) parentOfWrapper),
+          getNamesDeclaredByStmtList(wrapees))) {
+        return false;
+      }
+    }
+
     return true;
   }
 
+  static Set<String> getNamesDeclaredDirectlyByBlock(BlockStmt block) {
+    final List<Stmt> stmts = block.getStmts();
+    return getNamesDeclaredByStmtList(stmts);
+  }
+
+  private static Set<String> getNamesDeclaredByStmtList(List<Stmt> stmts) {
+    return stmts
+        .stream()
+        .filter(item -> item instanceof DeclarationStmt)
+        .map(item -> ((DeclarationStmt) item).getVariablesDeclaration().getDeclInfos())
+        .reduce(Collections.emptyList(), ListConcat::concatenate)
+        .stream()
+        .map(item -> item.getName())
+        .collect(Collectors.toSet());
+  }
+
 }

reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/LiveOutputVariableWriteReductionOpportunitiesTest.java

@@ -51,8 +51,28 @@ public void testLiveGLFragColorWriteOpportunity() throws Exception {
               .findOpportunities(MakeShaderJobFromFragmentShader.make(tu), new ReducerContext(false, null, null, null, true));
     assertEquals(1, ops.size());
     ops.get(0).applyReduction();
-    assertEquals(PrettyPrinterVisitor.prettyPrintAsString(ParseHelper.parse(reducedProgram)),
-          PrettyPrinterVisitor.prettyPrintAsString(tu));
+    CompareAsts.assertEqualAsts(reducedProgram, tu);
+  }
+
+  @Test
+  public void testLiveGLFragColorWriteOpportunityAtRootOfMain() throws Exception {
+    // Checks that we can eliminate a live color write if it is at the root of a function, i.e.
+    // not enclosed in any additional block.
+    final String backupName = Constants.GLF_OUT_VAR_BACKUP_PREFIX + OpenGlConstants.GL_FRAG_COLOR;
+    final String program = "void main() {"
+        + "  vec4 " + backupName + ";"
+        + "  " + backupName + " = " + OpenGlConstants.GL_FRAG_COLOR + ";"
+        + "  " + OpenGlConstants.GL_FRAG_COLOR + " = " + backupName + ";"
+        + "}";
+    final String reducedProgram = "void main() {"
+        + "}";
+    final TranslationUnit tu = ParseHelper.parse(program);
+    List<LiveOutputVariableWriteReductionOpportunity> ops =
+        LiveOutputVariableWriteReductionOpportunities
+            .findOpportunities(MakeShaderJobFromFragmentShader.make(tu), new ReducerContext(false, null, null, null, true));
+    assertEquals(1, ops.size());
+    ops.get(0).applyReduction();
+    CompareAsts.assertEqualAsts(reducedProgram, tu);
   }
 
   @Test
@@ -123,4 +143,4 @@ public void testOpportunityIsPresent3() throws Exception {
     CompareAsts.assertEqualAsts(expected, tu);
   }
 
-}
+}

reducer/src/test/java/com/graphicsfuzz/reducer/reductionopportunities/UnwrapReductionOpportunitiesTest.java

@@ -19,6 +19,7 @@
 import com.graphicsfuzz.common.ast.TranslationUnit;
 import com.graphicsfuzz.common.glslversion.ShadingLanguageVersion;
 import com.graphicsfuzz.common.tool.PrettyPrinterVisitor;
+import com.graphicsfuzz.common.util.CompareAsts;
 import com.graphicsfuzz.util.Constants;
 import com.graphicsfuzz.common.util.IRandom;
 import com.graphicsfuzz.common.util.IdGenerator;
@@ -30,7 +31,9 @@
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 public class UnwrapReductionOpportunitiesTest {
 
@@ -53,6 +56,8 @@ public void testEmptyBlock() throws Exception {
     List<UnwrapReductionOpportunity> ops = UnwrapReductionOpportunities.findOpportunities(MakeShaderJobFromFragmentShader.make(tu),
           new ReducerContext(false,
           ShadingLanguageVersion.GLSL_440, new SameValueRandom(false, 0), null, true));
+    // No opportunities because the inner block is empty.  Another reduction pass may be able to
+    // delete it, but it cannot be unwrapped.
     assertEquals(0, ops.size());
   }
 
@@ -66,6 +71,55 @@ public void testNestedEmptyBlocks() throws Exception {
     assertEquals(1, ops.size());
   }
 
+  @Test
+  public void testUnwrapWithDeclNoClash() throws Exception {
+    final String program = "void main() { int x; { int y; } }";
+    final String expected = "void main() { int x; int y; }";
+    final TranslationUnit tu = ParseHelper.parse(program);
+    List<UnwrapReductionOpportunity> ops = UnwrapReductionOpportunities.findOpportunities(MakeShaderJobFromFragmentShader.make(tu),
+        new ReducerContext(false,
+            ShadingLanguageVersion.GLSL_440, new SameValueRandom(false, 0), null, true));
+    assertEquals(1, ops.size());
+    ops.get(0).applyReduction();
+    CompareAsts.assertEqualAsts(expected, tu);
+  }
+
+  @Test
+  public void testNoUnwrapWithDeclClash() throws Exception {
+    final String program = "void main() { int x; { { int x; } x = 2; } }";
+    final String expected = "void main() { int x; { int x; } x = 2; }";
+    final TranslationUnit tu = ParseHelper.parse(program);
+    List<UnwrapReductionOpportunity> ops = UnwrapReductionOpportunities.findOpportunities(MakeShaderJobFromFragmentShader.make(tu),
+        new ReducerContext(false,
+            ShadingLanguageVersion.GLSL_440, new SameValueRandom(false, 0), null, true));
+    // The inner block cannot be unwrapped as it would change which variable 'x = 2' refers to.
+    assertEquals(1, ops.size());
+  }
+
+  @Test
+  public void testNoUnwrapWithDeclClash2() throws Exception {
+    final String program = "void main() { { int x; } float x; }";
+    final TranslationUnit tu = ParseHelper.parse(program);
+    List<UnwrapReductionOpportunity> ops = UnwrapReductionOpportunities.findOpportunities(MakeShaderJobFromFragmentShader.make(tu),
+        new ReducerContext(false,
+            ShadingLanguageVersion.GLSL_440, new SameValueRandom(false, 0), null, true));
+    assertEquals(0, ops.size());
+  }
+
+  @Test
+  public void testOneUnwrapDisablesAnother() throws Exception {
+    final String program = "void main() { { int x; } { float x; } }";
+    final TranslationUnit tu = ParseHelper.parse(program);
+    List<UnwrapReductionOpportunity> ops = UnwrapReductionOpportunities.findOpportunities(MakeShaderJobFromFragmentShader.make(tu),
+        new ReducerContext(false,
+            ShadingLanguageVersion.GLSL_440, new SameValueRandom(false, 0), null, true));
+    assertEquals(2, ops.size());
+    assertTrue(ops.get(0).preconditionHolds());
+    assertTrue(ops.get(1).preconditionHolds());
+    ops.get(0).applyReduction();
+    assertFalse(ops.get(1).preconditionHolds());
+  }
+
   @Test
   public void misc() throws Exception {
     final String shader = "void main()\n"