Fix samplerless image fetch
OpImageFetch takes an Image operand, not a SampledImage, and thus we
don't explicitly have a sampler descriptor. However, our
vk::SampledImageDescriptor structure includes both image view descriptor
data and sampler data, and we were relying on the sampler descriptor
data to be present for setting the addressing mode. Other state would
also inadvertently be affected.
This change ensures that the sampler descriptor for fetch operations is
null and we set the addressing mode to repeat but leave other unused
state at their defaults.
Bug: b/139401791
Change-Id: I9f1af35940c2fa9c7d30e771ebdb244f72f5bdbe
Reviewed-on: https://swiftshader-review.googlesource.com/c/SwiftShader/+/35348
Presubmit-Ready: Nicolas Capens <nicolascapens@google.com>
Kokoro-Presubmit: kokoro <noreply+kokoro@google.com>
Tested-by: Nicolas Capens <nicolascapens@google.com>
Reviewed-by: Chris Forbes <chrisforbes@google.com>
diff --git a/src/Pipeline/SpirvShader.cpp b/src/Pipeline/SpirvShader.cpp
index 5ec62c4..dc99bfa 100644
--- a/src/Pipeline/SpirvShader.cpp
+++ b/src/Pipeline/SpirvShader.cpp
@@ -4910,7 +4910,7 @@
{
Type::ID resultTypeId = insn.word(1);
Object::ID resultId = insn.word(2);
- Object::ID sampledImageId = insn.word(3);
+ Object::ID sampledImageId = insn.word(3); // For OpImageFetch this is just an Image, not a SampledImage.
Object::ID coordinateId = insn.word(4);
auto &resultType = getType(resultTypeId);
@@ -4928,6 +4928,14 @@
Pointer<Byte> sampler = samplerDescriptor + OFFSET(vk::SampledImageDescriptor, sampler); // vk::Sampler*
Pointer<Byte> texture = imageDescriptor + OFFSET(vk::SampledImageDescriptor, texture); // sw::Texture*
+ // Above we assumed that if the SampledImage operand is not the result of an OpSampledImage,
+ // it must be a combined image sampler loaded straight from the descriptor set. For OpImageFetch
+ // it's just an Image operand, so there's no sampler descriptor data.
+ if(getType(sampledImage.type).opcode() != spv::OpTypeSampledImage)
+ {
+ sampler = Pointer<Byte>(nullptr);
+ }
+
uint32_t imageOperands = spv::ImageOperandsMaskNone;
bool lodOrBias = false;
Object::ID lodOrBiasId = 0;
diff --git a/src/Pipeline/SpirvShader.hpp b/src/Pipeline/SpirvShader.hpp
index 49736c0..cdee2bf 100644
--- a/src/Pipeline/SpirvShader.hpp
+++ b/src/Pipeline/SpirvShader.hpp
@@ -1265,7 +1265,7 @@
// TODO(b/129523279): Eliminate conversion and use vk::Sampler members directly.
static sw::FilterType convertFilterMode(const vk::Sampler *sampler);
static sw::MipmapType convertMipmapMode(const vk::Sampler *sampler);
- static sw::AddressingMode convertAddressingMode(int coordinateIndex, VkSamplerAddressMode addressMode, VkImageViewType imageViewType);
+ static sw::AddressingMode convertAddressingMode(int coordinateIndex, const vk::Sampler *sampler, VkImageViewType imageViewType);
// Returns 0 when invalid.
static VkShaderStageFlagBits executionModelToStage(spv::ExecutionModel model);
diff --git a/src/Pipeline/SpirvShaderSampling.cpp b/src/Pipeline/SpirvShaderSampling.cpp
index 9013bf6..6b5afa2 100644
--- a/src/Pipeline/SpirvShaderSampling.cpp
+++ b/src/Pipeline/SpirvShaderSampling.cpp
@@ -34,9 +34,10 @@
SpirvShader::ImageSampler *SpirvShader::getImageSampler(uint32_t inst, vk::SampledImageDescriptor const *imageDescriptor, const vk::Sampler *sampler)
{
ImageInstruction instruction(inst);
- ASSERT(imageDescriptor->imageViewId != 0 && (sampler->id != 0 || instruction.samplerMethod == Fetch));
+ const auto samplerId = sampler ? sampler->id : 0;
+ ASSERT(imageDescriptor->imageViewId != 0 && (samplerId != 0 || instruction.samplerMethod == Fetch));
- vk::Device::SamplingRoutineCache::Key key = {inst, imageDescriptor->imageViewId, sampler->id};
+ vk::Device::SamplingRoutineCache::Key key = {inst, imageDescriptor->imageViewId, samplerId};
ASSERT(imageDescriptor->device);
@@ -59,34 +60,41 @@
Sampler samplerState = {};
samplerState.textureType = type;
samplerState.textureFormat = imageDescriptor->format;
- samplerState.textureFilter = (instruction.samplerMethod == Gather) ? FILTER_GATHER : convertFilterMode(sampler);
- samplerState.border = sampler->borderColor;
- samplerState.addressingModeU = convertAddressingMode(0, sampler->addressModeU, type);
- samplerState.addressingModeV = convertAddressingMode(1, sampler->addressModeV, type);
- samplerState.addressingModeW = convertAddressingMode(2, sampler->addressModeW, type);
+ samplerState.addressingModeU = convertAddressingMode(0, sampler, type);
+ samplerState.addressingModeV = convertAddressingMode(1, sampler, type);
+ samplerState.addressingModeW = convertAddressingMode(2, sampler, type);
samplerState.mipmapFilter = convertMipmapMode(sampler);
samplerState.swizzle = imageDescriptor->swizzle;
samplerState.gatherComponent = instruction.gatherComponent;
samplerState.highPrecisionFiltering = false;
- samplerState.compareEnable = (sampler->compareEnable == VK_TRUE);
- samplerState.compareOp = sampler->compareOp;
- samplerState.unnormalizedCoordinates = (sampler->unnormalizedCoordinates == VK_TRUE);
samplerState.largeTexture = (imageDescriptor->extent.width > SHRT_MAX) ||
(imageDescriptor->extent.height > SHRT_MAX) ||
(imageDescriptor->extent.depth > SHRT_MAX);
- if(sampler->ycbcrConversion)
+ if(sampler)
{
- samplerState.ycbcrModel = sampler->ycbcrConversion->ycbcrModel;
- samplerState.studioSwing = (sampler->ycbcrConversion->ycbcrRange == VK_SAMPLER_YCBCR_RANGE_ITU_NARROW);
- samplerState.swappedChroma = (sampler->ycbcrConversion->components.r != VK_COMPONENT_SWIZZLE_R);
- }
+ samplerState.textureFilter = (instruction.samplerMethod == Gather) ? FILTER_GATHER : convertFilterMode(sampler);
+ samplerState.border = sampler->borderColor;
- if(sampler->anisotropyEnable != VK_FALSE)
- {
- UNSUPPORTED("anisotropyEnable");
+ samplerState.mipmapFilter = convertMipmapMode(sampler);
+
+ samplerState.compareEnable = (sampler->compareEnable == VK_TRUE);
+ samplerState.compareOp = sampler->compareOp;
+ samplerState.unnormalizedCoordinates = (sampler->unnormalizedCoordinates == VK_TRUE);
+
+ if(sampler->ycbcrConversion)
+ {
+ samplerState.ycbcrModel = sampler->ycbcrConversion->ycbcrModel;
+ samplerState.studioSwing = (sampler->ycbcrConversion->ycbcrRange == VK_SAMPLER_YCBCR_RANGE_ITU_NARROW);
+ samplerState.swappedChroma = (sampler->ycbcrConversion->components.r != VK_COMPONENT_SWIZZLE_R);
+ }
+
+ if(sampler->anisotropyEnable != VK_FALSE)
+ {
+ UNSUPPORTED("anisotropyEnable");
+ }
}
routine = emitSamplerRoutine(instruction, samplerState);
@@ -249,6 +257,11 @@
sw::MipmapType SpirvShader::convertMipmapMode(const vk::Sampler *sampler)
{
+ if(!sampler)
+ {
+ return MIPMAP_POINT; // Samplerless operations (OpImageFetch) can take an integer Lod operand.
+ }
+
if(sampler->ycbcrConversion)
{
return MIPMAP_NONE; // YCbCr images can only have one mipmap level.
@@ -264,7 +277,7 @@
}
}
-sw::AddressingMode SpirvShader::convertAddressingMode(int coordinateIndex, VkSamplerAddressMode addressMode, VkImageViewType imageViewType)
+sw::AddressingMode SpirvShader::convertAddressingMode(int coordinateIndex, const vk::Sampler *sampler, VkImageViewType imageViewType)
{
switch(imageViewType)
{
@@ -342,6 +355,27 @@
return ADDRESSING_WRAP;
}
+ if(!sampler)
+ {
+ // OpImageFetch does not take a sampler descriptor, but still needs a valid,
+ // arbitrary addressing mode that prevents out-of-bounds accesses:
+ // "The value returned by a read of an invalid texel is undefined, unless that
+ // read operation is from a buffer resource and the robustBufferAccess feature
+ // is enabled. In that case, an invalid texel is replaced as described by the
+ // robustBufferAccess feature." - Vulkan 1.1
+
+ return ADDRESSING_WRAP;
+ }
+
+ VkSamplerAddressMode addressMode = VK_SAMPLER_ADDRESS_MODE_REPEAT;
+ switch(coordinateIndex)
+ {
+ case 0: addressMode = sampler->addressModeU; break;
+ case 1: addressMode = sampler->addressModeV; break;
+ case 2: addressMode = sampler->addressModeW; break;
+ default: UNSUPPORTED("coordinateIndex: %d", coordinateIndex);
+ }
+
switch(addressMode)
{
case VK_SAMPLER_ADDRESS_MODE_REPEAT: return ADDRESSING_WRAP;
diff --git a/src/Vulkan/vulkan.vcxproj b/src/Vulkan/vulkan.vcxproj
index 5a1fac8..dd4def0 100644
--- a/src/Vulkan/vulkan.vcxproj
+++ b/src/Vulkan/vulkan.vcxproj
@@ -80,7 +80,8 @@
mkdir "$(SolutionDir)build\$(Configuration)_$(Platform)\"
copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)out\$(Configuration)_$(Platform)\"
copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)build\$(Configuration)_$(Platform)\"
-IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\vulkan-1.dll")</Command>
+IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Debug" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Debug\vulkan-1.dll")
+IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Release" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Release\vulkan-1.dll")</Command>
</PostBuildEvent>
</ItemDefinitionGroup>
<ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
@@ -105,7 +106,8 @@
mkdir "$(SolutionDir)build\$(Configuration)_$(Platform)\"
copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)out\$(Configuration)_$(Platform)\"
copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)build\$(Configuration)_$(Platform)\"
-IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\vulkan-1.dll")</Command>
+IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Debug" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Debug\vulkan-1.dll")
+IF EXIST "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Release" (copy "$(OutDir)vk_swiftshader.dll" "$(SolutionDir)..\deqp\build\external\vulkancts\modules\vulkan\Release\vulkan-1.dll")</Command>
</PostBuildEvent>
</ItemDefinitionGroup>
<ItemGroup>
