Skip to content

Commit 6a7eae1

Browse files
KeenutsKeenuts
authored
[SPIR-V] Fix image write to unknown format (microsoft#6984)
By default, texture format is guessed from the type. But the `vk::image_format` attribute can be added to specify it. The `unknown` value was used to convey `no format selected`, and require the guess to happen. This made selecting `unknown` as value impossible. Once the optional added, we have a new issue: - The format setup relied on the legalizer. - load/stores are done using the default format, then we fix it, then we use the legalizer to propagate the format fix to all users. The capability visitor is running before the legalizer, meaning it can look at a non-fixed load, which still carries the old type. The way to solve this is to remove this logic, and move the capability addition/deletion to the capability_trimming pass, run after legalization. Fixes microsoft#6981 --------- Signed-off-by: Nathan Gauër <brioche@google.com>
1 parent ee1f013 commit 6a7eae1

File tree

7 files changed

+50
-38
lines changed

7 files changed

+50
-38
lines changed

‎tools/clang/include/clang/SPIRV/SpirvContext.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
#include <array>
1313
#include <limits>
14+
#include <optional>
1415

1516
#include "dxc/DXIL/DxilShaderModel.h"
1617
#include "clang/AST/DeclTemplate.h"
@@ -138,7 +139,7 @@ struct FunctionTypeMapInfo {
138139
struct VkImageFeatures {
139140
// True if it is a Vulkan "Combined Image Sampler".
140141
bool isCombinedImageSampler;
141-
spv::ImageFormat format; // SPIR-V image format.
142+
std::optional<spv::ImageFormat> format; // SPIR-V image format.
142143
};
143144

144145
// A struct that contains the information of a resource that will be used to
@@ -378,7 +379,7 @@ class SpirvContext {
378379
getVkImageFeaturesForSpirvVariable(const SpirvVariable *spvVar) {
379380
auto itr = spvVarToVkImageFeatures.find(spvVar);
380381
if (itr == spvVarToVkImageFeatures.end())
381-
return {false, spv::ImageFormat::Unknown};
382+
return {false, std::nullopt};
382383
return itr->second;
383384
}
384385

‎tools/clang/lib/SPIRV/CapabilityVisitor.cpp‎

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -439,36 +439,13 @@ bool CapabilityVisitor::visit(SpirvImageSparseTexelsResident *instr) {
439439
return true;
440440
}
441441

442-
namespace {
443-
bool isImageOpOnUnknownFormat(const SpirvImageOp *instruction) {
444-
if (!instruction->getImage() || !instruction->getImage()->getResultType()) {
445-
return false;
446-
}
447-
448-
const ImageType *imageType =
449-
dyn_cast<ImageType>(instruction->getImage()->getResultType());
450-
if (!imageType || imageType->getImageFormat() != spv::ImageFormat::Unknown) {
451-
return false;
452-
}
453-
454-
return imageType->getImageFormat() == spv::ImageFormat::Unknown;
455-
}
456-
} // namespace
457-
458442
bool CapabilityVisitor::visit(SpirvImageOp *instr) {
459443
addCapabilityForType(instr->getResultType(), instr->getSourceLocation(),
460444
instr->getStorageClass());
461445
if (instr->hasOffset() || instr->hasConstOffsets())
462446
addCapability(spv::Capability::ImageGatherExtended);
463447
if (instr->isSparse())
464448
addCapability(spv::Capability::SparseResidency);
465-
466-
if (isImageOpOnUnknownFormat(instr)) {
467-
addCapability(instr->isImageWrite()
468-
? spv::Capability::StorageImageWriteWithoutFormat
469-
: spv::Capability::StorageImageReadWithoutFormat);
470-
}
471-
472449
return true;
473450
}
474451

@@ -864,6 +841,8 @@ bool CapabilityVisitor::visit(SpirvModule *, Visitor::Phase phase) {
864841
// supports only some capabilities. This list should be expanded to match the
865842
// supported capabilities.
866843
addCapability(spv::Capability::MinLod);
844+
addCapability(spv::Capability::StorageImageWriteWithoutFormat);
845+
addCapability(spv::Capability::StorageImageReadWithoutFormat);
867846

868847
addExtensionAndCapabilitiesIfEnabled(
869848
Extension::EXT_fragment_shader_interlock,

‎tools/clang/lib/SPIRV/DeclResultIdMapper.cpp‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -517,9 +517,10 @@ SpirvLayoutRule getLayoutRuleForExternVar(QualType type,
517517
return SpirvLayoutRule::Void;
518518
}
519519

520-
spv::ImageFormat getSpvImageFormat(const VKImageFormatAttr *imageFormatAttr) {
520+
std::optional<spv::ImageFormat>
521+
getSpvImageFormat(const VKImageFormatAttr *imageFormatAttr) {
521522
if (imageFormatAttr == nullptr)
522-
return spv::ImageFormat::Unknown;
523+
return std::nullopt;
523524

524525
switch (imageFormatAttr->getImageFormat()) {
525526
case VKImageFormatAttr::unknown:
@@ -1235,14 +1236,13 @@ SpirvVariable *DeclResultIdMapper::createExternVar(const VarDecl *var,
12351236
VkImageFeatures vkImgFeatures = {
12361237
var->getAttr<VKCombinedImageSamplerAttr>() != nullptr,
12371238
getSpvImageFormat(var->getAttr<VKImageFormatAttr>())};
1238-
if (vkImgFeatures.format != spv::ImageFormat::Unknown) {
1239+
if (vkImgFeatures.format) {
12391240
// Legalization is needed to propagate the correct image type for
12401241
// instructions in addition to cases where the resource is assigned to
12411242
// another variable or function parameter
12421243
needsLegalization = true;
12431244
}
1244-
if (vkImgFeatures.isCombinedImageSampler ||
1245-
vkImgFeatures.format != spv::ImageFormat::Unknown) {
1245+
if (vkImgFeatures.isCombinedImageSampler || vkImgFeatures.format) {
12461246
spvContext.registerVkImageFeaturesForSpvVariable(varInstr, vkImgFeatures);
12471247
}
12481248

@@ -1256,8 +1256,9 @@ SpirvVariable *DeclResultIdMapper::createExternVar(const VarDecl *var,
12561256
}
12571257

12581258
if (hlsl::IsHLSLResourceType(type)) {
1259-
if (!areFormatAndTypeCompatible(vkImgFeatures.format,
1260-
hlsl::GetHLSLResourceResultType(type))) {
1259+
if (!areFormatAndTypeCompatible(
1260+
vkImgFeatures.format.value_or(spv::ImageFormat::Unknown),
1261+
hlsl::GetHLSLResourceResultType(type))) {
12611262
emitError("The image format and the sampled type are not compatible.\n"
12621263
"For the table of compatible types, see "
12631264
"https://docs.vulkan.org/spec/latest/appendices/"

‎tools/clang/lib/SPIRV/LowerTypeVisitor.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -198,15 +198,17 @@ bool LowerTypeVisitor::visitInstruction(SpirvInstruction *instr) {
198198
}
199199

200200
auto vkImgFeatures = spvContext.getVkImageFeaturesForSpirvVariable(var);
201-
if (vkImgFeatures.format != spv::ImageFormat::Unknown) {
201+
if (vkImgFeatures.format) {
202202
if (const auto *imageType = dyn_cast<ImageType>(resultType)) {
203-
resultType = spvContext.getImageType(imageType, vkImgFeatures.format);
203+
resultType =
204+
spvContext.getImageType(imageType, *vkImgFeatures.format);
204205
instr->setResultType(resultType);
205206
} else if (const auto *arrayType = dyn_cast<ArrayType>(resultType)) {
206207
if (const auto *imageType =
207208
dyn_cast<ImageType>(arrayType->getElementType())) {
208-
auto newImgType =
209-
spvContext.getImageType(imageType, vkImgFeatures.format);
209+
auto newImgType = spvContext.getImageType(
210+
imageType,
211+
vkImgFeatures.format.value_or(spv::ImageFormat::Unknown));
210212
resultType = spvContext.getArrayType(newImgType,
211213
arrayType->getElementCount(),
212214
arrayType->getStride());

‎tools/clang/test/CodeGenSPIRV/op.buffer.access.hlsl‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
// RUN: %dxc -T ps_6_0 -E main -fcgl %s -spirv | FileCheck %s
22

33
// CHECK: OpCapability ImageBuffer
4-
// CHECK: OpCapability StorageImageReadWithoutFormat
5-
4+
// CHECK-NOT: OpCapability StorageImageReadWithoutFormat
65

76
// CHECK: %type_buffer_image = OpTypeImage %int Buffer 2 0 0 1 R32i
87
// CHECK: %type_buffer_image_0 = OpTypeImage %uint Buffer 2 0 0 1 R32ui

‎tools/clang/test/CodeGenSPIRV/vk.attribute.image-format.unknown.hlsl‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// RUN: %dxc -T cs_6_7 -E main -spirv -fspv-target-env=vulkan1.3 %s
2+
3+
// CHECK: OpCapability StorageImageWriteWithoutFormat
4+
5+
// CHECK: %[[#image:]] = OpTypeImage %float 3D 2 0 0 2 Unknown
6+
[[vk::image_format("unknown")]] RWTexture3D<float32_t2> untypedImage;
7+
8+
[numthreads(8,8,8)]
9+
void main(uint32_t3 gl_GlobalInvocationID : SV_DispatchThreadID)
10+
{
11+
// CHECK: %[[#tmp:]] = OpLoad %[[#image]] %[[#]]
12+
// CHECK: OpImageWrite [[#tmp]] %[[#]] %[[#]] None
13+
untypedImage[gl_GlobalInvocationID] = float32_t2(4,5);
14+
}

‎tools/clang/test/CodeGenSPIRV/vk.attribute.image-format.unknown.read.hlsl‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// RUN: %dxc -T cs_6_7 -E main -spirv -fspv-target-env=vulkan1.3 %s
2+
3+
// CHECK: OpCapability StorageImageReadWithoutFormat
4+
5+
// CHECK: %[[#image:]] = OpTypeImage %float 1D 2 0 0 2 Unknown
6+
[[vk::image_format("unknown")]] RWTexture1D<float32_t2> untypedImage;
7+
RWStructuredBuffer<float32_t2> output;
8+
9+
[numthreads(8,8,8)]
10+
void main()
11+
{
12+
// CHECK: %[[#tmp:]] = OpLoad %[[#image]] %[[#]]
13+
// CHECK: OpImageRead %[[#]] [[#tmp]] %[[#]] None
14+
output[0] = untypedImage[0];
15+
}
16+

0 commit comments

Comments
 (0)