Vulkan: SPIR-V Gen: Fix conditionals with pruned blocks

Rewored the visitIfElse function to simplify the logic and correctly
handle if-else constructs where the true block is pruned.

Bug: angleproject:4889
Change-Id: Ib968a2fe65f4b6463158fd76e7d67757115ee832
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3046101
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Tim Van Patten <timvp@google.com>

src/compiler/translator/OutputSPIRV.cpp

@@ -4940,57 +4940,88 @@ bool OutputSPIRVTraverser::visitTernary(Visit visit, TIntermTernary *node)
 
 bool OutputSPIRVTraverser::visitIfElse(Visit visit, TIntermIfElse *node)
 {
-    if (visit == PreVisit)
+    // An if condition may or may not have an else block.  When both blocks are present, the
+    // translation is as follows:
+    //
+    // if (cond) { trueBody } else { falseBody }
+    //
+    //               // pre-if block
+    //       %cond = ...
+    //               OpSelectionMerge %merge None
+    //               OpBranchConditional %cond %true %false
+    //
+    //       %true = OpLabel
+    //               trueBody
+    //               OpBranch %merge
+    //
+    //      %false = OpLabel
+    //               falseBody
+    //               OpBranch %merge
+    //
+    //               // post-if block
+    //       %merge = OpLabel
+    //
+    // If the else block is missing, OpBranchConditional will simply jump to %merge on the false
+    // condition and the %false block is removed.  Due to the way ParseContext prunes compile-time
+    // constant conditionals, the if block itself may also be missing, which is treated similarly.
+
+    // It's simpler if this function performs the traversal.
+    ASSERT(visit == PreVisit);
+
+    // Visit the condition.
+    node->getCondition()->traverse(this);
+    const spirv::IdRef conditionValue =
+        accessChainLoad(&mNodeData.back(), node->getCondition()->getType(), nullptr);
+
+    // If both true and false blocks are missing, there's nothing to do.
+    if (node->getTrueBlock() == nullptr && node->getFalseBlock() == nullptr)
     {
-        // Don't add an entry to the stack.  The condition will create one, which we won't pop.
-        return true;
+        return false;
     }
 
-    const size_t lastChildIndex = getLastTraversedChildIndex(visit);
+    // Create a conditional with maximum 3 blocks, one for the true block (if any), one for the
+    // else block (if any), and one for the merge block.  getChildCount() works here as it
+    // produces an identical count.
+    mBuilder.startConditional(node->getChildCount(), false, false);
 
-    // If the condition was just visited, evaluate it and create the branch instructions.
-    if (lastChildIndex == 0)
+    // Generate the branch instructions.
+    const SpirvConditional *conditional = mBuilder.getCurrentConditional();
+
+    const spirv::IdRef mergeBlock = conditional->blockIds.back();
+    spirv::IdRef trueBlock        = mergeBlock;
+    spirv::IdRef falseBlock       = mergeBlock;
+
+    size_t nextBlockIndex = 0;
+    if (node->getTrueBlock())
     {
-        const spirv::IdRef conditionValue =
-            accessChainLoad(&mNodeData.back(), node->getCondition()->getType(), nullptr);
-
-        // Create a conditional with maximum 3 blocks, one for the true block (if any), one for the
-        // else block (if any), and one for the merge block.  getChildCount() works here as it
-        // produces an identical count.
-        mBuilder.startConditional(node->getChildCount(), false, false);
-
-        // Generate the branch instructions.
-        const SpirvConditional *conditional = mBuilder.getCurrentConditional();
-
-        const spirv::IdRef mergeBlock = conditional->blockIds.back();
-        spirv::IdRef trueBlock        = mergeBlock;
-        spirv::IdRef falseBlock       = mergeBlock;
-
-        size_t nextBlockIndex = 0;
-        if (node->getTrueBlock())
-        {
-            trueBlock = conditional->blockIds[nextBlockIndex++];
-        }
-        if (node->getFalseBlock())
-        {
-            falseBlock = conditional->blockIds[nextBlockIndex++];
-        }
-
-        mBuilder.writeBranchConditional(conditionValue, trueBlock, falseBlock, mergeBlock);
-        return true;
+        trueBlock = conditional->blockIds[nextBlockIndex++];
+    }
+    if (node->getFalseBlock())
+    {
+        falseBlock = conditional->blockIds[nextBlockIndex++];
     }
 
-    // Otherwise move on to the next block, inserting a branch to the merge block at the end of each
-    // block.
-    mBuilder.writeBranchConditionalBlockEnd();
+    mBuilder.writeBranchConditional(conditionValue, trueBlock, falseBlock, mergeBlock);
+
+    // Visit the true block, if any.
+    if (node->getTrueBlock())
+    {
+        node->getTrueBlock()->traverse(this);
+        mBuilder.writeBranchConditionalBlockEnd();
+    }
+
+    // Visit the false block, if any.
+    if (node->getFalseBlock())
+    {
+        node->getFalseBlock()->traverse(this);
+        mBuilder.writeBranchConditionalBlockEnd();
+    }
 
     // Pop from the conditional stack when done.
-    if (visit == PostVisit)
-    {
-        mBuilder.endConditional();
-    }
+    mBuilder.endConditional();
 
-    return true;
+    // Don't traverse the children, that's done already.
+    return false;
 }
 
 bool OutputSPIRVTraverser::visitSwitch(Visit visit, TIntermSwitch *node)

src/tests/deqp_support/deqp_gles2_test_expectations.txt

@@ -863,9 +863,6 @@
 5990 PIXEL4ORXL VULKAN : dEQP-GLES2.functional.vertex_arrays.single_attribute.usages.buffer_0_32_fixed2_vec2_static_draw_quads_256 = SKIP
 
 // Direct SPIR-V generation failures
-4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.discard.dynamic_loop_never = SKIP
-4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.discard.function_static_loop_never = SKIP
-4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.discard.static_loop_never = SKIP
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.array_arguments.copy_local_in_on_call_fragment = FAIL
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.array_arguments.copy_local_in_on_call_vertex = FAIL
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.array_arguments.copy_local_inout_on_call_fragment = FAIL
@@ -891,7 +888,6 @@
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.qualifiers.in_lowp_float_vertex = FAIL
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.qualifiers.in_lowp_int_fragment = FAIL
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.functions.qualifiers.in_lowp_int_vertex = FAIL
-4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.random.all_features.vertex.32 = SKIP
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.texture_functions.fragment.texture2dproj_vec4 = SKIP
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.texture_functions.fragment.texture2dproj_vec4_bias = SKIP
 4889 SPIRVGEN VULKAN : dEQP-GLES2.functional.shaders.texture_functions.vertex.texture2dproj_vec4 = SKIP

src/tests/gl_tests/GLSLTest.cpp

@@ -7999,6 +7999,47 @@ void main()
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test that if-else blocks whose contents get pruned due to compile-time constant conditions work.
+TEST_P(GLSLTest, IfElsePrunedBlocks)
+{
+    constexpr char kFS[] = R"(precision mediump float;
+uniform float u;
+void main()
+{
+    // if with only a pruned true block
+    if (u > 0.0)
+        if (false) discard;
+
+    // if with a pruned true block and a false block
+    if (u > 0.0)
+    {
+        if (false) discard;
+    }
+    else
+        ;
+
+    // if with a true block and a pruned false block
+    if (u > 0.0)
+        ;
+    else
+        if (false) discard;
+
+    // if with a pruned true block and a pruned false block
+    if (u > 0.0)
+    {
+        if (false) discard;
+    }
+    else
+        if (false) discard;
+
+    gl_FragColor = vec4(0, 1, 0, 1);
+})";
+
+    ANGLE_GL_PROGRAM(program, essl1_shaders::vs::Simple(), kFS);
+    drawQuad(program.get(), essl1_shaders::PositionAttrib(), 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Tests that PointCoord behaves the same betweeen a user FBO and the back buffer.
 TEST_P(GLSLTest, PointCoordConsistency)
 {