Implement VK_KHR_vulkan_memory_model
This is largely a no-op because CPUs typically have fully coherent
cache hierarchies (including on ccNUMA systems). Thus every write is
implicitly 'visible'. Atomic operations with memory-order semantics
likewise have system-wide 'scope'. And we don't support spare images yet
so 'availability' operations don't do anything.
Note that cache coherency would be lost if we were to use non-temporal
store instructions (e.g. MOVNTPS). A 'visibility' operation would
require inserting an SFENCE instruction.
The `VolatileTexel` image operand signifies that "This access cannot be
eliminated, duplicated, or combined with other accesses." There doesn't
appear to be dEQP-VK test coverage for it at this time, which makes it
non-trivial to optimally design how to handle it. It is therefore not
implemented at this time.
Bug: b/176819536
Tests: dEQP-VK.memory_model.*
Change-Id: Ifa97eed5a4506eef8ba77b8fb2042a0b1a624db5
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/56209
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Commit-Queue: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Pipeline/SpirvShader.cpp b/src/Pipeline/SpirvShader.cpp
index 6a0134f..264a471 100644
--- a/src/Pipeline/SpirvShader.cpp
+++ b/src/Pipeline/SpirvShader.cpp
@@ -419,6 +419,8 @@
case spv::CapabilityDeviceGroup: capabilities.DeviceGroup = true; break;
case spv::CapabilityMultiView: capabilities.MultiView = true; break;
case spv::CapabilityStencilExportEXT: capabilities.StencilExportEXT = true; break;
+ case spv::CapabilityVulkanMemoryModel: capabilities.VulkanMemoryModel = true; break;
+ case spv::CapabilityVulkanMemoryModelDeviceScope: capabilities.VulkanMemoryModelDeviceScope = true; break;
default:
UNSUPPORTED("Unsupported capability %u", insn.word(1));
}
@@ -428,7 +430,11 @@
break;
case spv::OpMemoryModel:
- break; // Memory model does not affect our code generation until we decide to do Vulkan Memory Model support.
+ {
+ addressingModel = static_cast<spv::AddressingModel>(insn.word(1));
+ memoryModel = static_cast<spv::MemoryModel>(insn.word(2));
+ }
+ break;
case spv::OpFunction:
{
@@ -745,7 +751,7 @@
case spv::OpExtension:
{
- auto ext = insn.string(1);
+ const char *ext = insn.string(1);
// Part of core SPIR-V 1.3. Vulkan 1.1 implementations must also accept the pre-1.3
// extension per Appendix A, `Vulkan Environment for SPIR-V`.
if(!strcmp(ext, "SPV_KHR_storage_buffer_storage_class")) break;
@@ -756,6 +762,7 @@
if(!strcmp(ext, "SPV_KHR_multiview")) break;
if(!strcmp(ext, "SPV_EXT_shader_stencil_export")) break;
if(!strcmp(ext, "SPV_KHR_float_controls")) break;
+ if(!strcmp(ext, "SPV_KHR_vulkan_memory_model")) break;
UNSUPPORTED("SPIR-V Extension: %s", ext);
}
break;
diff --git a/src/Pipeline/SpirvShader.hpp b/src/Pipeline/SpirvShader.hpp
index efdd11a..d549c0e 100644
--- a/src/Pipeline/SpirvShader.hpp
+++ b/src/Pipeline/SpirvShader.hpp
@@ -680,6 +680,8 @@
bool DeviceGroup : 1;
bool MultiView : 1;
bool StencilExportEXT : 1;
+ bool VulkanMemoryModel : 1;
+ bool VulkanMemoryModelDeviceScope : 1;
};
const Capabilities &getUsedCapabilities() const
@@ -885,17 +887,20 @@
Function::ID entryPoint;
spv::ExecutionModel executionModel = spv::ExecutionModelMax; // Invalid prior to OpEntryPoint parsing.
-
ExecutionModes executionModes = {};
- Analysis analysis = {};
Capabilities capabilities = {};
+ spv::AddressingModel addressingModel = spv::AddressingModelLogical;
+ spv::MemoryModel memoryModel = spv::MemoryModelSimple;
+ HandleMap<Extension> extensionsByID;
+ std::unordered_set<uint32_t> extensionsImported;
+
+ Analysis analysis = {};
+ mutable bool imageWriteEmitted = false;
+
HandleMap<Type> types;
HandleMap<Object> defs;
HandleMap<Function> functions;
std::unordered_map<StringID, String> strings;
- HandleMap<Extension> extensionsByID;
- std::unordered_set<uint32_t> extensionsImported;
- mutable bool imageWriteEmitted = false;
// DeclareType creates a Type for the given OpTypeX instruction, storing
// it into the types map. It is called from the analysis pass (constructor).
diff --git a/src/Pipeline/SpirvShaderControlFlow.cpp b/src/Pipeline/SpirvShaderControlFlow.cpp
index 98bb04c..2b55ca4 100644
--- a/src/Pipeline/SpirvShaderControlFlow.cpp
+++ b/src/Pipeline/SpirvShaderControlFlow.cpp
@@ -623,8 +623,8 @@
{
auto executionScope = spv::Scope(GetConstScalarInt(insn.word(1)));
auto semantics = spv::MemorySemanticsMask(GetConstScalarInt(insn.word(3)));
- // TODO: We probably want to consider the memory scope here. For now,
- // just always emit the full fence.
+ // TODO(b/176819536): We probably want to consider the memory scope here.
+ // For now, just always emit the full fence.
Fence(semantics);
switch(executionScope)
@@ -713,15 +713,6 @@
}
}
-void SpirvShader::Fence(spv::MemorySemanticsMask semantics) const
-{
- if(semantics == spv::MemorySemanticsMaskNone)
- {
- return; // no-op
- }
- rr::Fence(MemoryOrder(semantics));
-}
-
void SpirvShader::Yield(YieldResult res) const
{
rr::Yield(RValue<Int>(int(res)));
diff --git a/src/Pipeline/SpirvShaderImage.cpp b/src/Pipeline/SpirvShaderImage.cpp
index 68727b1..d66a1a2 100644
--- a/src/Pipeline/SpirvShaderImage.cpp
+++ b/src/Pipeline/SpirvShaderImage.cpp
@@ -225,6 +225,34 @@
imageOperands &= ~spv::ImageOperandsSignExtendMask;
}
+ [[maybe_unused]] spv::Scope scope = spv::ScopeCrossDevice; // "Whilst the CrossDevice scope is defined in SPIR-V, it is disallowed in Vulkan."
+
+ if(imageOperands & spv::ImageOperandsMakeTexelAvailableMask)
+ {
+ scope = static_cast<spv::Scope>(insn.word(operandsIndex));
+ operandsIndex += 1;
+ imageOperands &= ~spv::ImageOperandsMakeTexelAvailableMask;
+ }
+
+ if(imageOperands & spv::ImageOperandsMakeTexelVisibleMask)
+ {
+ scope = static_cast<spv::Scope>(insn.word(operandsIndex));
+ operandsIndex += 1;
+ imageOperands &= ~spv::ImageOperandsMakeTexelVisibleMask;
+ }
+
+ if(imageOperands & spv::ImageOperandsNonPrivateTexelMask)
+ {
+ imageOperands &= ~spv::ImageOperandsNonPrivateTexelMask;
+ }
+
+ if(imageOperands & spv::ImageOperandsVolatileTexelMask)
+ {
+ UNIMPLEMENTED("b/176819536");
+ imageOperands &= ~spv::ImageOperandsVolatileTexelMask;
+ }
+
+ // There should be no remaining image operands.
if(imageOperands != 0)
{
UNSUPPORTED("Image operands 0x%08X", imageOperands);
diff --git a/src/Pipeline/SpirvShaderMemory.cpp b/src/Pipeline/SpirvShaderMemory.cpp
index 18ed889..8439011 100644
--- a/src/Pipeline/SpirvShaderMemory.cpp
+++ b/src/Pipeline/SpirvShaderMemory.cpp
@@ -279,8 +279,8 @@
SpirvShader::EmitResult SpirvShader::EmitMemoryBarrier(InsnIterator insn, EmitState *state) const
{
auto semantics = spv::MemorySemanticsMask(GetConstScalarInt(insn.word(2)));
- // TODO: We probably want to consider the memory scope here. For now,
- // just always emit the full fence.
+ // TODO(b/176819536): We probably want to consider the memory scope here.
+ // For now, just always emit the full fence.
Fence(semantics);
return EmitResult::Continue;
}
@@ -421,13 +421,21 @@
}
}
+void SpirvShader::Fence(spv::MemorySemanticsMask semantics) const
+{
+ if(semantics != spv::MemorySemanticsMaskNone)
+ {
+ rr::Fence(MemoryOrder(semantics));
+ }
+}
+
std::memory_order SpirvShader::MemoryOrder(spv::MemorySemanticsMask memorySemantics)
{
- auto control = static_cast<uint32_t>(memorySemantics) & static_cast<uint32_t>(
- spv::MemorySemanticsAcquireMask |
- spv::MemorySemanticsReleaseMask |
- spv::MemorySemanticsAcquireReleaseMask |
- spv::MemorySemanticsSequentiallyConsistentMask);
+ uint32_t control = static_cast<uint32_t>(memorySemantics) & static_cast<uint32_t>(
+ spv::MemorySemanticsAcquireMask |
+ spv::MemorySemanticsReleaseMask |
+ spv::MemorySemanticsAcquireReleaseMask |
+ spv::MemorySemanticsSequentiallyConsistentMask);
switch(control)
{
case spv::MemorySemanticsMaskNone: return std::memory_order_relaxed;
@@ -437,7 +445,7 @@
case spv::MemorySemanticsSequentiallyConsistentMask: return std::memory_order_acq_rel; // Vulkan 1.1: "SequentiallyConsistent is treated as AcquireRelease"
default:
// "it is invalid for more than one of these four bits to be set:
- // Acquire, Release, AcquireRelease, or SequentiallyConsistent."
+ // Acquire, Release, AcquireRelease, or SequentiallyConsistent."
UNREACHABLE("MemorySemanticsMask: %x", int(control));
return std::memory_order_acq_rel;
}
diff --git a/src/Vulkan/VkPhysicalDevice.cpp b/src/Vulkan/VkPhysicalDevice.cpp
index 109e6aa..8169fed 100644
--- a/src/Vulkan/VkPhysicalDevice.cpp
+++ b/src/Vulkan/VkPhysicalDevice.cpp
@@ -268,9 +268,9 @@
template<typename T>
static void getPhysicalDeviceVulkanMemoryModelFeatures(T *features)
{
- features->vulkanMemoryModel = VK_FALSE;
- features->vulkanMemoryModelDeviceScope = VK_FALSE;
- features->vulkanMemoryModelAvailabilityVisibilityChains = VK_FALSE;
+ features->vulkanMemoryModel = VK_TRUE;
+ features->vulkanMemoryModelDeviceScope = VK_TRUE;
+ features->vulkanMemoryModelAvailabilityVisibilityChains = VK_TRUE;
}
template<typename T>
diff --git a/src/Vulkan/libVulkan.cpp b/src/Vulkan/libVulkan.cpp
index a925849..2f8da98 100644
--- a/src/Vulkan/libVulkan.cpp
+++ b/src/Vulkan/libVulkan.cpp
@@ -429,6 +429,7 @@
{ { VK_KHR_COPY_COMMANDS_2_EXTENSION_NAME, VK_KHR_COPY_COMMANDS_2_SPEC_VERSION } },
{ { VK_KHR_SWAPCHAIN_MUTABLE_FORMAT_EXTENSION_NAME, VK_KHR_SWAPCHAIN_MUTABLE_FORMAT_SPEC_VERSION } },
{ { VK_KHR_FORMAT_FEATURE_FLAGS_2_EXTENSION_NAME, VK_KHR_FORMAT_FEATURE_FLAGS_2_SPEC_VERSION } },
+ { { VK_KHR_VULKAN_MEMORY_MODEL_EXTENSION_NAME, VK_KHR_VULKAN_MEMORY_MODEL_SPEC_VERSION } },
};
static uint32_t numSupportedExtensions(const ExtensionProperties *extensionProperties, uint32_t extensionPropertiesCount)
@@ -940,6 +941,7 @@
case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_UNIFORM_BUFFER_STANDARD_LAYOUT_FEATURES:
case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SHADER_SUBGROUP_EXTENDED_TYPES_FEATURES:
case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_4444_FORMATS_FEATURES_EXT:
+ case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_VULKAN_MEMORY_MODEL_FEATURES:
break;
default:
// "the [driver] must skip over, without processing (other than reading the sType and pNext members) any structures in the chain with sType values not defined by [supported extenions]"
