Vulkan: Respect short-circuit more safely

Previously, if the LHS of `&&`/`||` or true/false expression of `?:`
didn't have side effects, non-short-circuiting SPIR-V instructions were
generated.  This is not completely safe, as an expression without side
effect might still be invalid to execute, such as loading from an array
with an out-of-bounds index.

Bug: angleproject:465831139
Change-Id: I5c865919d7a36523ba25ea50151a9451aef46e36
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/7228207
Commit-Queue: Yuxin Hu <[email protected]>
Auto-Submit: Shahbaz Youssefi <[email protected]>
Reviewed-by: Yuxin Hu <[email protected]>
Commit-Queue: Shahbaz Youssefi <[email protected]>

Author: ShabbyX

src/compiler/translator/spirv/OutputSPIRV.cpp

@@ -2341,6 +2341,60 @@ void OutputSPIRVTraverser::visitArrayLength(TIntermUnary *node)
     nodeDataInitRValue(&mNodeData.back(), castResultId, intTypeId);
 }
 
+// If an expression is short-circuited, it must not be executed.  However, in some cases there is
+// nothing to execute, such as constants, variables etc.  Notably, hasSideEffects() is not
+// a sufficient check, because it could include read-only operations that are out of bounds, despite
+// not having any side effects.
+bool IsSafeToExecuteInShortCircuit(TIntermTyped *node)
+{
+    // Constants and symbols are safe to execute.
+    if (node->getAsConstantUnion() || node->getAsSymbolNode())
+    {
+        return true;
+    }
+
+    // Swizzle is safe if the operand is safe.
+    {
+        TIntermSwizzle *asSwizzle = node->getAsSwizzleNode();
+        if (asSwizzle)
+        {
+            return IsSafeToExecuteInShortCircuit(asSwizzle->getOperand());
+        }
+    }
+
+    // Indexing a struct or interface block is safe to execute, as long as no array index is in the
+    // access chain.
+    {
+        TIntermBinary *asBinary = node->getAsBinaryNode();
+        if (asBinary != nullptr)
+        {
+            return (asBinary->getOp() == EOpIndexDirectInterfaceBlock ||
+                    asBinary->getOp() == EOpIndexDirectStruct) &&
+                   IsSafeToExecuteInShortCircuit(asBinary->getLeft());
+        }
+    }
+
+    // Constructors are safe as long as every member is safe.
+    {
+        TIntermAggregate *asAggregate = node->getAsAggregate();
+        if (asAggregate != nullptr && asAggregate->getOp() == EOpConstruct)
+        {
+            for (TIntermNode *component : *asAggregate->getSequence())
+            {
+                if (!IsSafeToExecuteInShortCircuit(component->getAsTyped()))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+    }
+
+    // Assume everything else is unsafe.  This includes safe but complex expressions that are better
+    // off not getting executed anyway.
+    return false;
+}
+
 bool IsShortCircuitNeeded(TIntermOperator *node)
 {
     TOperator op = node->getOp();
@@ -2353,12 +2407,9 @@ bool IsShortCircuitNeeded(TIntermOperator *node)
 
     ASSERT(node->getChildCount() == 2);
 
-    // If the right hand side does not have side effects, short-circuiting is unnecessary.
-    // TODO: experiment with the performance of OpLogicalAnd/Or vs short-circuit based on the
-    // complexity of the right hand side expression.  We could potentially only allow
-    // OpLogicalAnd/Or if the right hand side is a constant or an access chain and have more complex
-    // expressions be placed inside an if block.  http://anglebug.com/40096715
-    return node->getChildNode(1)->getAsTyped()->hasSideEffects();
+    // If the right hand side is not safe to execute, short-circuiting is needed
+    // For example: is_in_bounds(index) && access(data[index])
+    return !IsSafeToExecuteInShortCircuit(node->getChildNode(1)->getAsTyped());
 }
 
 using WriteUnaryOp      = void (*)(spirv::Blob *blob,
@@ -5300,8 +5351,8 @@ bool OutputSPIRVTraverser::visitTernary(Visit visit, TIntermTernary *node)
     // prior to 1.4 requires the type to be either scalar or vector.
     const TType &type   = node->getType();
     bool canUseOpSelect = (type.isScalar() || type.isVector() || mCompileOptions.emitSPIRV14) &&
-                          !node->getTrueExpression()->hasSideEffects() &&
-                          !node->getFalseExpression()->hasSideEffects();
+                          IsSafeToExecuteInShortCircuit(node->getTrueExpression()) &&
+                          IsSafeToExecuteInShortCircuit(node->getFalseExpression());
 
     // Don't use OpSelect on buggy drivers.  Technically this is only needed if the two sides don't
     // have matching use of RelaxedPrecision, but not worth being precise about it.

src/tests/angle_end2end_tests_expectations.txt

@@ -1153,6 +1153,7 @@
 42266091 WIN INTEL : *EmulateCopyTexImage2DFromRenderbuffers* = SKIP
 
 42266092 WIN SWIFTSHADER : VulkanPerformanceCounterTest.EndXfbAfterRenderPassClosed/* = SKIP
+466331749 WIN SWIFTSHADER : GLSLTest_ES31.ShortCircuitOutOfBoundsAccess/* = SKIP
 
 // Flat shading bug with lighting
 42264726 VULKAN : LightsTest.FlatLitMesh/* = SKIP
@@ -1812,6 +1813,12 @@
 42264369 ANDROID GLES : TextureBufferTestES31.DrawIncompleteNonZeroTexture/* = SKIP
 42264369 ANDROID GLES : TextureBufferTestES31.DrawCompleteZeroTexture/* = SKIP
 42264369 ANDROID GLES : TextureBufferTestES31.DrawCompleteNonZeroTexture/* = SKIP
+42264369 ANDROID GLES : TextureBufferTestES31.TexBufferDrawTwice/* = SKIP
+42264369 ANDROID GLES : TextureBufferTestES31.UseAsUBOThenUpdateThenAsTextureBuffer/* = SKIP
+42264369 ANDROID GLES : TextureBufferTestES31.MapTextureBufferInvalidateThenWrite/* = SKIP
+42264369 ANDROID GLES : ComputeShaderTest.ImageBufferMapWrite/* = SKIP
+42264369 ANDROID GLES : ComputeShaderTest.ImageBufferMapWriteAndBufferSubData/* = SKIP
+42264369 ANDROID GLES : GLSLTest_ES31.ShortCircuitOutOfBoundsAccess/* = SKIP
 
 // WebGPU failures
 // All tests are flaky on Linux Intel due to X errors.

src/tests/gl_tests/ComputeShaderTest.cpp

@@ -5096,10 +5096,6 @@ TEST_P(ComputeShaderTest, ImageBufferMapWrite)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
 
-    // Claims to support GL_OES_texture_buffer, but fails compilation of shader because "extension
-    // 'GL_OES_texture_buffer' is not supported".  http://anglebug.com/42264369
-    ANGLE_SKIP_TEST_IF(IsQualcomm() && IsOpenGLES());
-
     constexpr char kComputeImageBuffer[] = R"(#version 310 es
 #extension GL_OES_texture_buffer : require
 layout(local_size_x=1, local_size_y=1, local_size_z=1) in;
@@ -5167,10 +5163,6 @@ TEST_P(ComputeShaderTest, ImageBufferMapWriteAndBufferSubData)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
 
-    // Claims to support GL_OES_texture_buffer, but fails compilation of shader because "extension
-    // 'GL_OES_texture_buffer' is not supported".  http://anglebug.com/42264369
-    ANGLE_SKIP_TEST_IF(IsQualcomm() && IsOpenGLES());
-
     // http://anglebug.com/42265043. Known bug.
     ANGLE_SKIP_TEST_IF(IsVulkan());
 

src/tests/gl_tests/GLSLTest.cpp

@@ -3820,6 +3820,99 @@ void main()
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::white);
 }
 
+// Test that short circuit doesn't evaluate out of bounds expressions.
+TEST_P(GLSLTest_ES31, ShortCircuitOutOfBoundsAccess)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
+
+    // Note that the uniform doesn't need to be set, and will contain the default value of false.
+    constexpr char kCS[] = R"(#version 310 es
+#extension GL_OES_texture_buffer : require
+layout(local_size_x = 128, local_size_y = 1, local_size_z = 1) in;
+
+uniform bool falseValue;
+uniform uint zero;
+
+layout(binding = 0) readonly buffer Input
+{
+    uint inData[];
+};
+
+layout(binding = 1, std430) writeonly buffer Output
+{
+    uint outData[];
+};
+
+layout(r32ui, binding = 0) uniform highp uimageBuffer image;
+
+void main()
+{
+    outData[0] = falseValue ? inData[123456] : 1u;
+    outData[1] = falseValue ? inData[~zero] : 2u;
+    outData[2] = falseValue ? inData[123456] : 3u;
+    outData[3] = !falseValue ? 4u : inData[~zero];
+    outData[4] = !falseValue ? 5u : imageLoad(image, 1234567).x;
+    outData[5] = falseValue ? imageLoad(image, int(~zero)).x : 6u;
+
+    bool eval6 = falseValue && bool(inData[123456]);
+    bool eval7 = falseValue && bool(inData[~zero]);
+    bool eval8 = !falseValue || bool(inData[123456]);
+    bool eval9 = !falseValue || bool(inData[~zero]);
+    bool eval10 = falseValue && bool(imageLoad(image, 1234567).x);
+    bool eval11 = !falseValue || bool(imageLoad(image, int(~zero)).x);
+
+    outData[6] = eval6 ? 1234u : 7u;
+    outData[7] = eval7 ? 2345u : 8u;
+    outData[8] = eval8 ? 9u : 3456u;
+    outData[9] = eval9 ? 10u : 4567u;
+    outData[10] = eval10 ? 5678u : 11u;
+    outData[11] = eval11 ? 12u : 6789u;
+})";
+
+    ANGLE_GL_COMPUTE_PROGRAM(program, kCS);
+
+    GLBuffer input;
+    glBindBuffer(GL_SHADER_STORAGE_BUFFER, input);
+    glBufferData(GL_SHADER_STORAGE_BUFFER, sizeof(uint32_t), nullptr, GL_STATIC_DRAW);
+    glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 0, input);
+
+    constexpr std::array<uint32_t, 12> kInitialData = {};
+
+    GLBuffer output;
+    glBindBuffer(GL_SHADER_STORAGE_BUFFER, output);
+    glBufferData(GL_SHADER_STORAGE_BUFFER, sizeof(kInitialData), kInitialData.data(),
+                 GL_STATIC_DRAW);
+    glBindBufferBase(GL_SHADER_STORAGE_BUFFER, 1, output);
+
+    GLBuffer imageBufferStorage;
+    GLTexture imageBuffer;
+
+    glBindBuffer(GL_TEXTURE_BUFFER, imageBufferStorage);
+    glBufferData(GL_TEXTURE_BUFFER, sizeof(uint32_t), nullptr, GL_STATIC_DRAW);
+
+    glBindTexture(GL_TEXTURE_BUFFER, imageBuffer);
+    glTexBufferEXT(GL_TEXTURE_BUFFER, GL_R32UI, imageBufferStorage);
+    glBindImageTexture(0, imageBuffer, 0, GL_FALSE, 0, GL_READ_WRITE, GL_R32UI);
+    ASSERT_GL_NO_ERROR();
+
+    glUseProgram(program);
+    glDispatchCompute(1, 1, 1);
+    glMemoryBarrier(GL_BUFFER_UPDATE_BARRIER_BIT);
+    glFinish();
+
+    std::array<uint32_t, 12> readback = {};
+    void *mapped =
+        glMapBufferRange(GL_SHADER_STORAGE_BUFFER, 0, sizeof(kInitialData), GL_MAP_READ_BIT);
+    memcpy(readback.data(), mapped, sizeof(kInitialData));
+    glUnmapBuffer(GL_SHADER_STORAGE_BUFFER);
+    ASSERT_GL_NO_ERROR();
+
+    for (size_t i = 0; i < kInitialData.size(); ++i)
+    {
+        EXPECT_EQ(readback[i], i + 1) << i;
+    }
+}
+
 // Test that nesting ternary and short-circuitting operators work.
 TEST_P(GLSLTest, NestedTernaryAndShortCircuit)
 {

src/tests/gl_tests/TextureTest.cpp

@@ -16692,10 +16692,6 @@ TEST_P(TextureBufferTestES31, TexBufferDrawTwice)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
 
-    // TODO(http://anglebug.com/42264369): Claims to support GL_OES_texture_buffer, but fails
-    // compilation of shader because "extension 'GL_OES_texture_buffer' is not supported".
-    ANGLE_SKIP_TEST_IF(IsQualcomm() && IsOpenGLES());
-
     const std::array<GLColor, 1> kTexData = {GLColor::red};
 
     GLBuffer buffer;
@@ -16983,10 +16979,6 @@ TEST_P(TextureBufferTestES31, UseAsUBOThenUpdateThenAsTextureBuffer)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
 
-    // Claims to support GL_OES_texture_buffer, but fails compilation of shader because "extension
-    // 'GL_OES_texture_buffer' is not supported".  http://anglebug.com/42264369
-    ANGLE_SKIP_TEST_IF(IsQualcomm() && IsOpenGLES());
-
     const std::array<GLColor, 4> kInitialData = {GLColor::red, GLColor::red, GLColor::red,
                                                  GLColor::red};
     const std::array<GLColor, 4> kUpdateData  = {GLColor::blue, GLColor::blue, GLColor::blue,
@@ -17053,9 +17045,6 @@ TEST_P(TextureBufferTestES31, MapTextureBufferInvalidateThenWrite)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_OES_texture_buffer"));
 
-    // TODO(http://anglebug.com/42264369): Claims to support GL_OES_texture_buffer, but fails
-    // compilation of shader because "extension 'GL_OES_texture_buffer' is not supported".
-    ANGLE_SKIP_TEST_IF(IsQualcomm() && IsOpenGLES());
     // TODO(http://anglebug.com/42264910): The OpenGL backend doesn't correctly handle texture
     // buffers being invalidated when mapped.
     ANGLE_SKIP_TEST_IF(IsOpenGL());