Default to masking stores in helper invocations
The Vulkan spec states that "Stores and atomics performed by helper
invocations must not have any effect on memory except for the Function
and Private storage classes". Thus the switch statement which checks the
storage class of stores in helper invocations must default to not
causing any effect.
The Function and Private storage classes are the exception, and the
discussion at https://gitlab.khronos.org/Tracker/vk-gl-cts/-/issues/4060
strongly supports that the Output storage class should get the same
treatment.
Note that this code previously also considered Output to have effect, so
this change is the closest thing to a refactoring and we can alter the
behaviour later if deemed incorrect after all.
The method was renamed and meaning inverted for additional clarity of
its semantics.
Bug: b/253701784
Change-Id: I0315d3e6ed4571307f1d6d2c5a33bff5d6f096d0
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/69228
Tested-by: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
diff --git a/src/Pipeline/SpirvShader.hpp b/src/Pipeline/SpirvShader.hpp
index 072b036..6043714 100644
--- a/src/Pipeline/SpirvShader.hpp
+++ b/src/Pipeline/SpirvShader.hpp
@@ -866,9 +866,6 @@
// Creates an Object for the instruction's result in 'defs'.
void DefineResult(const InsnIterator &insn);
- // Output storage buffers and images should not be affected by helper invocations
- static bool StoresInHelperInvocation(spv::StorageClass storageClass);
-
using InterfaceVisitor = std::function<void(Decorations const, AttribType)>;
void VisitInterface(Object::ID id, const InterfaceVisitor &v) const;
@@ -977,6 +974,7 @@
// Returns 0 when invalid.
static VkShaderStageFlagBits executionModelToStage(spv::ExecutionModel model);
+ static bool StoresInHelperInvocationsHaveNoEffect(spv::StorageClass storageClass);
static bool IsExplicitLayout(spv::StorageClass storageClass);
static bool IsTerminator(spv::Op opcode);
};
diff --git a/src/Pipeline/SpirvShaderMemory.cpp b/src/Pipeline/SpirvShaderMemory.cpp
index b2c45c0..bcbff15 100644
--- a/src/Pipeline/SpirvShaderMemory.cpp
+++ b/src/Pipeline/SpirvShaderMemory.cpp
@@ -108,7 +108,7 @@
auto robustness = shader.getOutOfBoundsBehavior(pointerId, routine->pipelineLayout);
SIMD::Int mask = activeLaneMask();
- if(!shader.StoresInHelperInvocation(pointerTy.storageClass))
+ if(shader.StoresInHelperInvocationsHaveNoEffect(pointerTy.storageClass))
{
mask = mask & storesAndAtomicsMask();
}
@@ -527,17 +527,18 @@
}
}
-bool SpirvShader::StoresInHelperInvocation(spv::StorageClass storageClass)
+bool SpirvShader::StoresInHelperInvocationsHaveNoEffect(spv::StorageClass storageClass)
{
switch(storageClass)
{
- case spv::StorageClassUniform:
- case spv::StorageClassStorageBuffer:
- case spv::StorageClassPhysicalStorageBuffer:
- case spv::StorageClassImage:
- return false;
+ // "Stores and atomics performed by helper invocations must not have any effect on memory..."
default:
return true;
+ // "...except for the Function and Private storage classes".
+ case spv::StorageClassFunction:
+ case spv::StorageClassPrivate:
+ case spv::StorageClassOutput: // TODO(b/253701784): We assume Output should be treated as Private.
+ return false;
}
}
