Implement shaderStorageImageMultisample support
This fixes a bug in the descriptor data where the sample pitch was set
to the layer pitch for array images (it should always be the slice
pitch), and the descriptor's slice pitch (which stores the layer pitch
in case of array images), was taking the sample count into account
twice.
Note that while the image robustness extensions still allow read
operations to return undefined values "if only the sample index was
invalid", so clamping that value would suffice (as currently done in our
OpImageFetch implementation), write operations always have to be no-op.
This change also 'nullifies' the OpImageRead instructions for invalid
sample indices.
Bug: b/163142358
Tests: dEQP-VK.*
Change-Id: I9d816c249e14a17722d29f56015c8dc8cc7d51d4
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/47548
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alexis Hétu <sugoi@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
diff --git a/src/Pipeline/SpirvShader.cpp b/src/Pipeline/SpirvShader.cpp
index 854515c..e308e42 100644
--- a/src/Pipeline/SpirvShader.cpp
+++ b/src/Pipeline/SpirvShader.cpp
@@ -361,15 +361,17 @@
{
case spv::CapabilityMatrix: capabilities.Matrix = true; break;
case spv::CapabilityShader: capabilities.Shader = true; break;
+ case spv::CapabilityStorageImageMultisample: capabilities.StorageImageMultisample = true; break;
case spv::CapabilityClipDistance: capabilities.ClipDistance = true; break;
case spv::CapabilityCullDistance: capabilities.CullDistance = true; break;
+ case spv::CapabilityImageCubeArray: capabilities.ImageCubeArray = true; break;
case spv::CapabilityInputAttachment: capabilities.InputAttachment = true; break;
case spv::CapabilitySampled1D: capabilities.Sampled1D = true; break;
case spv::CapabilityImage1D: capabilities.Image1D = true; break;
- case spv::CapabilityImageCubeArray: capabilities.ImageCubeArray = true; break;
case spv::CapabilitySampledBuffer: capabilities.SampledBuffer = true; break;
case spv::CapabilitySampledCubeArray: capabilities.SampledCubeArray = true; break;
case spv::CapabilityImageBuffer: capabilities.ImageBuffer = true; break;
+ case spv::CapabilityImageMSArray: capabilities.ImageMSArray = true; break;
case spv::CapabilityStorageImageExtendedFormats: capabilities.StorageImageExtendedFormats = true; break;
case spv::CapabilityImageQuery: capabilities.ImageQuery = true; break;
case spv::CapabilityDerivativeControl: capabilities.DerivativeControl = true; break;
diff --git a/src/Pipeline/SpirvShader.hpp b/src/Pipeline/SpirvShader.hpp
index 29248fa..b8f4f40 100644
--- a/src/Pipeline/SpirvShader.hpp
+++ b/src/Pipeline/SpirvShader.hpp
@@ -569,15 +569,17 @@
{
bool Matrix : 1;
bool Shader : 1;
+ bool StorageImageMultisample : 1;
bool ClipDistance : 1;
bool CullDistance : 1;
+ bool ImageCubeArray : 1;
bool InputAttachment : 1;
bool Sampled1D : 1;
bool Image1D : 1;
- bool ImageCubeArray : 1;
bool SampledBuffer : 1;
bool SampledCubeArray : 1;
bool ImageBuffer : 1;
+ bool ImageMSArray : 1;
bool StorageImageExtendedFormats : 1;
bool ImageQuery : 1;
bool DerivativeControl : 1;
diff --git a/src/Pipeline/SpirvShaderImage.cpp b/src/Pipeline/SpirvShaderImage.cpp
index 4ce3532..59316c7 100644
--- a/src/Pipeline/SpirvShaderImage.cpp
+++ b/src/Pipeline/SpirvShaderImage.cpp
@@ -115,7 +115,7 @@
return EmitImageSample({ variant, Grad }, insn, state);
}
else
- UNSUPPORTED("Image Operands %x", imageOperands);
+ UNSUPPORTED("Image operands 0x%08X", imageOperands);
return EmitResult::Continue;
}
@@ -234,7 +234,7 @@
if(imageOperands != 0)
{
- UNSUPPORTED("Image operand %x", imageOperands);
+ UNSUPPORTED("Image operands 0x%08X", imageOperands);
}
}
@@ -558,10 +558,12 @@
ptrOffset += SIMD::Int(routine->viewID) * slicePitch;
}
+ SIMD::Int n = 0;
if(sampleId.value())
{
Operand sample(this, state, sampleId);
- ptrOffset += sample.Int(0) * samplePitch;
+ n = sample.Int(0);
+ ptrOffset += n * samplePitch;
}
// If the out-of-bounds behavior is set to nullify, then each coordinate must be tested individually.
@@ -581,7 +583,13 @@
{
auto depth = *Pointer<UInt>(descriptor + OFFSET(vk::StorageImageDescriptor, extent.depth));
auto arrayLayers = *Pointer<UInt>(descriptor + OFFSET(vk::StorageImageDescriptor, arrayLayers));
- oobMask |= As<SIMD::Int>(CmpNLT(As<SIMD::UInt>(w), SIMD::UInt(depth * arrayLayers)));
+ oobMask |= As<SIMD::Int>(CmpNLT(As<SIMD::UInt>(w), SIMD::UInt(depth * arrayLayers))); // TODO: Precompute extent. 3D image can't have layers.
+ }
+
+ if(sampleId.value())
+ {
+ SIMD::UInt sampleCount = *Pointer<UInt>(descriptor + OFFSET(vk::StorageImageDescriptor, sampleCount));
+ oobMask |= As<SIMD::Int>(CmpNLT(As<SIMD::UInt>(n), sampleCount));
}
constexpr int32_t OOB_OFFSET = 0x7FFFFFFF - 16; // SIMD pointer offsets are signed 32-bit, so this is the largest offset (for 16-byte texels).
@@ -605,7 +613,7 @@
if(insn.wordCount() > 5)
{
int operand = 6;
- auto imageOperands = insn.word(5);
+ uint32_t imageOperands = insn.word(5);
if(imageOperands & spv::ImageOperandsSampleMask)
{
sampleId = insn.word(operand++);
@@ -613,7 +621,10 @@
}
// Should be no remaining image operands.
- ASSERT(!imageOperands);
+ if(imageOperands != 0)
+ {
+ UNSUPPORTED("Image operands 0x%08X", imageOperands);
+ }
}
ASSERT(imageType.definition.opcode() == spv::OpTypeImage);
@@ -990,8 +1001,24 @@
ASSERT(imageType.definition.opcode() == spv::OpTypeImage);
- // TODO(b/131171141): Not handling any image operands yet.
- ASSERT(insn.wordCount() == 4);
+ Object::ID sampleId = 0;
+
+ if(insn.wordCount() > 4)
+ {
+ int operand = 5;
+ uint32_t imageOperands = insn.word(4);
+ if(imageOperands & spv::ImageOperandsSampleMask)
+ {
+ sampleId = insn.word(operand++);
+ imageOperands &= ~spv::ImageOperandsSampleMask;
+ }
+
+ // Should be no remaining image operands.
+ if(imageOperands != 0)
+ {
+ UNSUPPORTED("Image operands 0x%08X", (int)imageOperands);
+ }
+ }
auto coordinate = Operand(this, state, insn.word(2));
auto texel = Operand(this, state, insn.word(3));
@@ -1176,7 +1203,7 @@
// - https://www.khronos.org/registry/vulkan/specs/1.2/html/chap16.html#textures-output-coordinate-validation
auto robustness = OutOfBoundsBehavior::Nullify;
- auto texelPtr = GetTexelAddress(state, imageBase, imageSizeInBytes, coordinate, imageType, binding, texelSize, 0, false, robustness);
+ auto texelPtr = GetTexelAddress(state, imageBase, imageSizeInBytes, coordinate, imageType, binding, texelSize, sampleId, false, robustness);
// Scatter packed texel data.
// TODO(b/160531165): Provide scatter abstractions for various element sizes.
@@ -1235,6 +1262,7 @@
ASSERT(getType(resultType.element).opcode() == spv::OpTypeInt);
auto coordinate = Operand(this, state, insn.word(4));
+ Object::ID sampleId = insn.word(5);
Pointer<Byte> binding = state->getPointer(imageId).base;
Pointer<Byte> imageBase = *Pointer<Pointer<Byte>>(binding + OFFSET(vk::StorageImageDescriptor, ptr));
@@ -1244,7 +1272,7 @@
// TODO(b/162327166): Only perform bounds checks when VK_EXT_image_robustness is enabled.
auto robustness = OutOfBoundsBehavior::Nullify;
- auto ptr = GetTexelAddress(state, imageBase, imageSizeInBytes, coordinate, imageType, binding, sizeof(uint32_t), 0, false, robustness);
+ auto ptr = GetTexelAddress(state, imageBase, imageSizeInBytes, coordinate, imageType, binding, sizeof(uint32_t), sampleId, false, robustness);
state->createPointer(resultId, ptr);
diff --git a/src/Vulkan/VkDescriptorSetLayout.cpp b/src/Vulkan/VkDescriptorSetLayout.cpp
index f987562..27fbde8 100644
--- a/src/Vulkan/VkDescriptorSetLayout.cpp
+++ b/src/Vulkan/VkDescriptorSetLayout.cpp
@@ -435,10 +435,10 @@
descriptor[i].ptr = imageView->getOffsetPointer({ 0, 0, 0 }, VK_IMAGE_ASPECT_COLOR_BIT, 0, 0);
descriptor[i].extent = imageView->getMipLevelExtent(0);
descriptor[i].rowPitchBytes = imageView->rowPitchBytes(VK_IMAGE_ASPECT_COLOR_BIT, 0);
- descriptor[i].samplePitchBytes = imageView->getSubresourceRange().layerCount > 1
- ? imageView->layerPitchBytes(VK_IMAGE_ASPECT_COLOR_BIT)
- : imageView->slicePitchBytes(VK_IMAGE_ASPECT_COLOR_BIT, 0);
- descriptor[i].slicePitchBytes = descriptor[i].samplePitchBytes * imageView->getSampleCount();
+ descriptor[i].samplePitchBytes = imageView->slicePitchBytes(VK_IMAGE_ASPECT_COLOR_BIT, 0);
+ descriptor[i].slicePitchBytes = imageView->getSubresourceRange().layerCount > 1
+ ? imageView->layerPitchBytes(VK_IMAGE_ASPECT_COLOR_BIT)
+ : imageView->slicePitchBytes(VK_IMAGE_ASPECT_COLOR_BIT, 0);
descriptor[i].arrayLayers = imageView->getSubresourceRange().layerCount;
descriptor[i].sampleCount = imageView->getSampleCount();
descriptor[i].sizeInBytes = static_cast<int>(imageView->getSizeInBytes());
@@ -448,10 +448,10 @@
{
descriptor[i].stencilPtr = imageView->getOffsetPointer({ 0, 0, 0 }, VK_IMAGE_ASPECT_STENCIL_BIT, 0, 0);
descriptor[i].stencilRowPitchBytes = imageView->rowPitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT, 0);
- descriptor[i].stencilSamplePitchBytes = (imageView->getSubresourceRange().layerCount > 1)
- ? imageView->layerPitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT)
- : imageView->slicePitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT, 0);
- descriptor[i].stencilSlicePitchBytes = descriptor[i].stencilSamplePitchBytes * imageView->getSampleCount();
+ descriptor[i].stencilSamplePitchBytes = imageView->slicePitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT, 0);
+ descriptor[i].stencilSlicePitchBytes = (imageView->getSubresourceRange().layerCount > 1)
+ ? imageView->layerPitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT)
+ : imageView->slicePitchBytes(VK_IMAGE_ASPECT_STENCIL_BIT, 0);
}
}
}
diff --git a/src/Vulkan/VkDescriptorSetLayout.hpp b/src/Vulkan/VkDescriptorSetLayout.hpp
index 7ab3439..6c7e159 100644
--- a/src/Vulkan/VkDescriptorSetLayout.hpp
+++ b/src/Vulkan/VkDescriptorSetLayout.hpp
@@ -59,7 +59,7 @@
void *ptr;
VkExtent3D extent;
int rowPitchBytes;
- int slicePitchBytes;
+ int slicePitchBytes; // Layer pitch in case of array image
int samplePitchBytes;
int arrayLayers;
int sampleCount;
@@ -67,7 +67,7 @@
void *stencilPtr;
int stencilRowPitchBytes;
- int stencilSlicePitchBytes;
+ int stencilSlicePitchBytes; // Layer pitch in case of array image
int stencilSamplePitchBytes;
ImageView *memoryOwner; // Pointer to the view which owns the memory used by the descriptor set
diff --git a/src/Vulkan/VkPhysicalDevice.cpp b/src/Vulkan/VkPhysicalDevice.cpp
index f53c326..137a095 100644
--- a/src/Vulkan/VkPhysicalDevice.cpp
+++ b/src/Vulkan/VkPhysicalDevice.cpp
@@ -98,7 +98,7 @@
VK_FALSE, // shaderTessellationAndGeometryPointSize
VK_FALSE, // shaderImageGatherExtended
VK_TRUE, // shaderStorageImageExtendedFormats
- VK_FALSE, // shaderStorageImageMultisample
+ VK_TRUE, // shaderStorageImageMultisample
VK_FALSE, // shaderStorageImageReadWithoutFormat
VK_FALSE, // shaderStorageImageWriteWithoutFormat
VK_TRUE, // shaderUniformBufferArrayDynamicIndexing
@@ -295,7 +295,7 @@
sampleCounts, // sampledImageIntegerSampleCounts
sampleCounts, // sampledImageDepthSampleCounts
sampleCounts, // sampledImageStencilSampleCounts
- VK_SAMPLE_COUNT_1_BIT, // storageImageSampleCounts (unsupported)
+ sampleCounts, // storageImageSampleCounts
1, // maxSampleMaskWords
VK_FALSE, // timestampComputeAndGraphics
60, // timestampPeriod
