[spirv] Add vk::image_format attribute for Buffers, RWBuffers and RWTextures #3395

seppala2 · 2021-01-26T11:26:49Z

According to Vulkan specification when using OpImageRead/OpImageWrite, the OpTypeImage (Buffers, RWBuffers, RWTextures) must have a format that matches the format on the API side, unless the StorageImageReadWithoutFormat/StorageImageWriteWithoutFormat is added and Unknown is used as the format.

This pull request addressess #2498 for the format part by adding an attribute [[vk::image_format("<image format as spelled in SPIR-V spec>")]]. Example of the syntax:

[[vk::image_format("rgba8")]]
RWBuffer<float4> Buf;

[[vk::image_format("rg16f")]]
RWTexture2D<float2> Tex;

RWTexture2D<float2> Tex2; // Works like before

The image_format only applies to global variables of type Buffer, RWBuffer, RWTexture. For variables and function parameters it is propagated by the inlining pass in legalization. This required a small change to one of the passes in SPIRV-Tools, that should be also checked by someone more familiar with the codebase: KhronosGroup/SPIRV-Tools#4126

Note that this does not fix the handling of unspecified format (that case still works like before, using R32f, etc. based on the type in shader), although it should be still fixed to add the
StorageImageReadWithoutFormat and/or StorageImageWriteWithoutFormat and use Undefined. But I think the ability to specify the format is more urgent.

Design note from Jaebaek:
Since the image_format attribute only applies to global variables, under the DXC architecture
only DeclResultIdMapper can check the attribute when it handles VarDecls. It means
we have to pass the image_format information to LowerTypeVisitor because it cannot access to
VarDecl. In order to pass the image_format, we use SpirvContext that can be accessed by
SpirvEmitter and all visitors. We use SpirvVariable to spv::ImageFormat mapping because the
attribute only applies to global variables (not to image types).
See how we use llvm::DenseMap<const SpirvVariable *, spv::ImageFormat> spvVarToImageFormat.

ghost · 2021-01-26T11:27:03Z

All CLA requirements met.

AppVeyorBot · 2021-01-26T12:15:43Z

✅ Build DirectXShaderCompiler 1.0.4186 completed (commit 97a531956e by @seppala2)

jaebaek

Thank you for working on this issue. Sorry for this late review.

I think we have to submit this PR after you submit the SPIRV-Tools PR because we want to add unit tests for this change.
Could you please add unit tests after submitting the SPIRV-Tools PR and updating the SPIRV-Tools in this PR?

you can add your tests in CodeGenSpirvTest.cpp
after adding HLSL files in CodeGenSPIRV directory - for example, type.texture.hlsl
you can test the unit test by running ninja && ./bin/clang-spirv-tests --spirv-test-root ../../tools/clang/test/CodeGenSPIRV/ (see ubuntu test or windows test)

If you have any difficulties to add unit tests, please let me know. I can help.

Thanks!

jaebaek · 2021-02-18T22:35:35Z

tools/clang/include/clang/Basic/Attr.td

+                           "R16Snorm", "R8Snorm", "Rgba32i", "Rgba16i", "Rgba8i", "R32i",
+                           "Rg32i", "Rg16i", "Rg8i", "R16i", "R8i", "Rgba32ui", "Rgba16ui", "Rgba8ui",
+                           "R32ui", "Rgb10a2ui", "Rg32ui", "Rg16ui", "Rg8ui", "R16ui",
+                           "R8ui", "R64ui", "R64i"]>];


Can you use lower cases for all the keywords here?

I agree that it looks a bit weird with camel case, but I chose to match with the identifiers in SPIR-V specification. Lower-case match of SPIR-V spec formats could be fine otherwise, but at least the format R11fG11fB10f could be less readable: "r11fg11fb10f". In that case, maybe changing to a completely custom style with underscores, like "r11g11b10_f", "rgb10a2_ui", "r32_ui" etc. would be better, not even trying to match the SPIR-V specification?

I think changing the naming by adding _ is not a good idea. It would require developers to know what that magic string is, and where the underscore should be and they'd have to look it up every time, and they'll not be happy :)

I think Jaebaek's comment is reasonable in the sense that it'll make developer's lives easier, and they won't get a compile error for case sensitivity.

In my opinion the compiler should accept both the text style that matches the SPIR-V spec exactly (e.g. Rgba16Snorm), and also accept all lowercase text (e.g. rgba16snorm). And the SPIR-V backend "should just do the right thing" either way. e.g.

case VKImageFormatAttr::Rgba16Snorm: case VKImageFormatAttr::Rgba16SnormLowercase: spvFormat = spv::ImageFormat::Rgba16Snorm; break;

Good point. I am just worried that people will be confused .. what would be correct one among Rgba32ui, RGBA32UI, RGBA32ui, and rgba32ui. Basically if we use only lower case characters it aligns with other HLSL keywords.

In addition, we have to give a description about the format keywords:

diff --git a/docs/SPIR-V.rst b/docs/SPIR-V.rst index fd07db8aa..950d064c4 100644 --- a/docs/SPIR-V.rst +++ b/docs/SPIR-V.rst @@ -782,6 +782,73 @@ are translated into SPIR-V ``OpTypeImage``, with parameters: The meanings of the headers in the above table is explained in ``OpTypeImage`` of the SPIR-V spec. +Vulkan specific Image Formats +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Since HLSL lacks the syntax for fully specifying image formats for textures in +SPIR-V, we introduce ``[[vk::image_format("FORMAT")]]`` attribute for texture types. +For example, + +.. code:: hlsl + [[vk::image_format("rgba8")]] + RWBuffer<float4> Buf; + + [[vk::image_format("rg16f")]] + RWTexture2D<float2> Tex; + + RWTexture2D<float2> Tex2; // Works like before + +``rgba8`` means ``Rgba8`` `SPIR-V Image Format <https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#_a_id_image_format_a_image_format>`_. +The following table lists the mapping between ``FORMAT`` of +``[[vk::image_format("FORMAT")]]`` and its corresponding SPIR-V Image Format. + +======================= ============================================ + FORMAT SPIR-V Image Format +======================= ============================================ +``unknown`` ``Unknown`` +``rgba32f`` ``Rgba32f`` +``rgba16f`` ``Rgba16f`` +``r32f`` ``R32f`` +``rgba8`` ``Rgba8`` +``rgba8snorm`` ``Rgba8Snorm`` +``rg32f`` ``Rg32f`` +``rg16f`` ``Rg16f`` +``r11g11b10f`` ``R11fG11fB10f`` +``r16f`` ``R16f`` +``rgba16`` ``Rgba16`` +``rgb10a2`` ``Rgb10A2`` +``rg16`` ``Rg16`` +``rg8`` ``Rg8`` +``r16`` ``R16`` +``r8`` ``R8`` +``rgba16snorm`` ``Rgba16Snorm`` +``rg16snorm`` ``Rg16Snorm`` +``rg8snorm`` ``Rg8Snorm`` +``r16snorm`` ``R16Snorm`` +``r8snorm`` ``R8Snorm`` +``rgba32i`` ``Rgba32i`` +``rgba16i`` ``Rgba16i`` +``rgba8i`` ``Rgba8i`` +``r32i`` ``R32i`` +``rg32i`` ``Rg32i`` +``rg16i`` ``Rg16i`` +``rg8i`` ``Rg8i`` +``r16i`` ``R16i`` +``r8i`` ``R8i`` +``rgba32ui`` ``Rgba32ui`` +``rgba16ui`` ``Rgba16ui`` +``rgba8ui`` ``Rgba8ui`` +``r32ui`` ``R32ui`` +``rgb10a2ui`` ``Rgb10a2ui`` +``rg32ui`` ``Rg32ui`` +``rg16ui`` ``Rg16ui`` +``rg8ui`` ``Rg8ui`` +``r16ui`` ``R16ui`` +``r8ui`` ``R8ui`` +``r64ui`` ``R64ui`` +``r64i`` ``R64i`` +======================= ============================================ + Constant/Texture/Structured/Byte Buffers ----------------------------------------

For the specific case of R11fG11fB10f, I prefer r11g11b10f that describes sizes in bits of each channel with their types (float) clearly and shortly. What do you think?

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

tools/clang/lib/SPIRV/LowerTypeVisitor.h

ehsannas

@seppala2 Thanks for your contributions! The SPIRV-Tools PR has landed already, which is great!

I just have a couple of minor comments, and one major concern: about where image format is stored. I honestly don't expect you to fix this part (we can directly push updates to this PR). I'd like to hear @jaebaek 's opinion on this part too. Maybe I'm too paranoid about it.

Again, thanks for your work!

ehsannas · 2021-02-19T16:46:22Z

tools/clang/include/clang/Basic/Attr.td

+                           "R16Snorm", "R8Snorm", "Rgba32i", "Rgba16i", "Rgba8i", "R32i",
+                           "Rg32i", "Rg16i", "Rg8i", "R16i", "R8i", "Rgba32ui", "Rgba16ui", "Rgba8ui",
+                           "R32ui", "Rgb10a2ui", "Rg32ui", "Rg16ui", "Rg8ui", "R16ui",
+                           "R8ui", "R64ui", "R64i"]>];


I think changing the naming by adding _ is not a good idea. It would require developers to know what that magic string is, and where the underscore should be and they'd have to look it up every time, and they'll not be happy :)

I think Jaebaek's comment is reasonable in the sense that it'll make developer's lives easier, and they won't get a compile error for case sensitivity.

In my opinion the compiler should accept both the text style that matches the SPIR-V spec exactly (e.g. Rgba16Snorm), and also accept all lowercase text (e.g. rgba16snorm). And the SPIR-V backend "should just do the right thing" either way. e.g.

case VKImageFormatAttr::Rgba16Snorm: case VKImageFormatAttr::Rgba16SnormLowercase: spvFormat = spv::ImageFormat::Rgba16Snorm; break;

ehsannas · 2021-02-19T16:59:56Z

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -847,6 +985,8 @@ SpirvVariable *DeclResultIdMapper::createExternVar(const VarDecl *var) {
      type, storageClass, var->hasAttr<HLSLPreciseAttr>(), name, llvm::None,
      loc);
  varInstr->setLayoutRule(rule);
+  handleImageFormat(var, varInstr);


IMO it's better to untangle the logic (the helper function should just convert an attribute to an spv::ImageFormat). Meaning this line should be replaced with:

varInstr->setImageFormat(getSpvImageFormat(var->getAttr<VKImageFormatAttr>());

now getSpvImageFormat requires no knowledge of SpirvVariable or SpirvInstruction class.

ehsannas · 2021-02-19T17:31:59Z

tools/clang/include/clang/SPIRV/SpirvInstruction.h

@@ -241,6 +244,7 @@ class SpirvInstruction {
  bool containsAlias;

  spv::StorageClass storageClass;
+  spv::ImageFormat imageFormat;


Based on the SPIR-V spec, the ImageFormat is only used by the Image Type. (no SPIR-V instruction directly uses the image format).

Therefore putting this information inside the SpirvInstruction class may not be the right thing to do. Ideally it should be placed inside the ImageType class. In fact, this class already has a member for the image format. It just needs to be populated correctly.

The fact that you had to add // Propagate image format to the result type of load code to manually propagate the information after OpLoad is a side effect of this. (ideally you shouldn't need to have a special case code for OpLoad. Are there other special cases that are missed?).

Having said all of this, the place where we set the image format in the image type is in the LowerTypeVisitor, and when we reach LowerTypeVisitor, we have already lost the vk::image_format attribute because we don't have access to Decls inside LowerTypeVisitor. @jaebaek, please think about this more carefully and provide a solution.

P.S. If we think putting this information inside the SpirvInstruction is the way to go, the propagation of the image format in the types should be done in a separate visitor pass before LowerTypeVisitor.

Regarding ImageType, the format parsed from the attribute is already put there, in LowerTypeVisitor:

const auto format = explicitImageFormat == spv::ImageFormat::Unknown ? translateSampledTypeToImageFormat(sampledType, srcLoc) : explicitImageFormat; return spvContext.getImageType( lowerType(getElementType(astContext, sampledType), rule, /*isRowMajor*/ llvm::None, srcLoc), dim, ImageType::WithDepth::Unknown, isArray, /*isMultiSampled=*/false, /*sampled=*/ImageType::WithSampler::No, format);

where I just added the explicitImageFormat.

The problem is that the proper format was not not known at that point, since the type is added while visiting the instruction (OpVariable for the global Buffer/RWTexture definition). So the most obvious way to pass that information was to put it inside the SpirvInstruction as some kind of metadata for the type required by that instruction.

So basically I put the imageformat into SpirvInstruction to denote the required image format for OpVariable (if it's a Buffer or RWTexture), so that it can be fetched when constructing the ImageType object in LowerTypeVisitor.cpp. But I see that the putting it inside SpirvInstruction may be a bit confusing since the type is not really part of the instruction.

ehsannas · 2021-02-19T17:37:01Z

tools/clang/lib/SPIRV/LowerTypeVisitor.cpp

@@ -60,11 +60,19 @@ bool LowerTypeVisitor::visitInstruction(SpirvInstruction *instr) {
  const QualType astType = instr->getAstResultType();
  const SpirvType *hybridType = instr->getResultType();

+  spv::ImageFormat format = instr->getImageFormat();
+  // Propagate image format to the result type of load
+  if (instr->getopcode() == spv::Op::OpLoad) {


this code makes me a bit nervous. Are there other special cases to consider? Either way the propagation of this information should be done inside a separate visitor pass, not inside LowerTypeVisitor.cpp

I looked into this a bit more and realized that this Propagation to OpLoad is unnecessary and works fine without it. So basically this special case can and should be removed.

The propagation of the defined type is not a special case, but more like a requirement for all stores and loads with this change (requiring unlimited number of propagations because the variable can be stored multiple times). But legalization in SPIRV-Tools fixes all of those cases, and even this single propagation step does not need to be here in the codegen.

jaebaek · 2021-02-19T19:18:31Z

Thank you for the detailed code review @ehsannas.
After reading comments from Ehsan, I realize this change has a few non-trivial design issues.

@seppala2 if it is ok to you, I think I should prepare a patch and give it to you so that you can apply my patch. I will get back to you after preparing it.

seppala2 · 2021-02-20T19:00:10Z

@jaebaek Sure, thanks.

jaebaek · 2021-02-25T03:25:56Z

@seppala2 I added a few commits on top of yours: https://github.com/jaebaek/DirectXShaderCompiler/tree/vkimg. If it is ok, could you please update your PR with my commits?

After updating your PR, please add comments if some changes from my updates have some issues.

Or if you want me to just open a new PR, please let me know.

jaebaek · 2021-02-25T15:49:15Z

Never mind. I just add my commits here. @ehsannas could you please review the code?

- Syntax: [[vk::image_format("Rgba16f")]] RWBuffer<float4>; - The format string matches the syntax in SPIR-V specification - Only applies to global resource variables, the format propagates to parameters and variables - To allow the propagation, a change in the SPIRV-Tools legalization pass is required

- Caused an existing SPIR-V test to fail

…tion

AppVeyorBot · 2021-02-25T16:46:24Z

✅ Build DirectXShaderCompiler 1.0.4407 completed (commit 5bce16e262 by @jaebaek)

seppala2 · 2021-02-25T20:25:53Z

@jaebaek thanks, I pulled the changes and tried it out, it seems to work as expected

ehsannas

Nice! can you please add one more test in which we CHECK the output after legalization?
Thanks

jaebaek · 2021-02-26T20:18:42Z

Added one more test. Thanks!

AppVeyorBot · 2021-02-26T23:04:54Z

✅ Build DirectXShaderCompiler 1.0.4427 completed (commit 2d4e0c4457 by @jaebaek)

This commit aim to fix microsoft#4773. In SPIR-V, image type must be specified, and GLSL has the syntax for it. In HLSL, we don't, until microsoft#3395. When this attribute is not specified, we decided to guess the type, not using Unknown anymore. But the type guessing was a bit too optimistic, and thus allowed a 3 component layout to be upgraded to 4 component layout. Changing the guessing function to either give a close type, or return Unknown, adding the capability on load/store to the variable.

This commit aim to fix #4773. In SPIR-V, image type must be specified, and GLSL has the syntax for it. In HLSL, we don't, until #3395. When this attribute is not specified, we decided to guess the type, not using Unknown anymore. But the type guessing was a bit too optimistic, and thus allowed a 3 component layout to be upgraded to 4 component layout. Changing the guessing function to either give a close type, or return Unknown, adding the capability on load/store to the variable.

This was referenced Jan 26, 2021

spirv-opt: Allow image type mismatch in GenerateCopy KhronosGroup/SPIRV-Tools#4126

Merged

[SPIRV] HLSL lacks syntax for fully specifying formats for RWTexture2D in SPIR-V #2498

Closed

jaebaek self-requested a review February 3, 2021 19:09

jaebaek self-assigned this Feb 3, 2021

jaebaek added the spirv Work related to SPIR-V label Feb 3, 2021

jaebaek reviewed Feb 18, 2021

View reviewed changes

ehsannas suggested changes Feb 19, 2021

View reviewed changes

seppala2 and others added 8 commits February 25, 2021 10:50

Fix missing delimiter in warn_attribute_wrong_decl_type

155893a

- Caused an existing SPIR-V test to fail

Add document

f1fa84c

Update submodules

c8e2047

Use lower cases for Vulkan image formats

574e8a0

Let SpirvContext keep image format of variable intead of SpirvInstruc…

693a14e

…tion

Clean up

c8373c3

Add unit test

caab937

jaebaek force-pushed the vk-image-format branch from 2acaf48 to caab937 Compare February 25, 2021 15:56

ehsannas reviewed Feb 26, 2021

View reviewed changes

Add more unit test

7bb6766

ehsannas approved these changes Feb 26, 2021

View reviewed changes

jaebaek merged commit 03cc4f8 into microsoft:master Mar 1, 2021

nyorain mentioned this pull request May 12, 2021

[SPIR-V] Invalid OpLoad when using vk::image_format #3767

Closed

Dredhog mentioned this pull request Mar 17, 2022

[SPIR-V] The vk::image_format attribute no longer works (even on the dedicated DXC tests) #4334

Closed

Keenuts mentioned this pull request Dec 12, 2022

spirv: keep image format to unknown if it is. #4868

Merged

s-perron mentioned this pull request Oct 9, 2024

Proposal for mapping resource attributes to DXIL and SPIR-V llvm/wg-hlsl#76

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[spirv] Add vk::image_format attribute for Buffers, RWBuffers and RWTextures #3395

[spirv] Add vk::image_format attribute for Buffers, RWBuffers and RWTextures #3395

seppala2 commented Jan 26, 2021 •

edited by jaebaek

Loading

ghost commented Jan 26, 2021 •

edited by ghost

Loading

AppVeyorBot commented Jan 26, 2021

jaebaek left a comment •

edited

Loading

jaebaek Feb 18, 2021

seppala2 Feb 19, 2021

ehsannas Feb 19, 2021

jaebaek Feb 19, 2021

ehsannas left a comment

ehsannas Feb 19, 2021

ehsannas Feb 19, 2021

ehsannas Feb 19, 2021

seppala2 Feb 20, 2021 •

edited

Loading

ehsannas Feb 19, 2021

seppala2 Feb 20, 2021

jaebaek commented Feb 19, 2021

seppala2 commented Feb 20, 2021

jaebaek commented Feb 25, 2021 •

edited

Loading

jaebaek commented Feb 25, 2021

AppVeyorBot commented Feb 25, 2021

seppala2 commented Feb 25, 2021

ehsannas left a comment

jaebaek commented Feb 26, 2021

AppVeyorBot commented Feb 26, 2021

[spirv] Add vk::image_format attribute for Buffers, RWBuffers and RWTextures #3395

[spirv] Add vk::image_format attribute for Buffers, RWBuffers and RWTextures #3395

Conversation

seppala2 commented Jan 26, 2021 • edited by jaebaek Loading

ghost commented Jan 26, 2021 • edited by ghost Loading

AppVeyorBot commented Jan 26, 2021

jaebaek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehsannas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seppala2 Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaebaek commented Feb 19, 2021

seppala2 commented Feb 20, 2021

jaebaek commented Feb 25, 2021 • edited Loading

jaebaek commented Feb 25, 2021

AppVeyorBot commented Feb 25, 2021

seppala2 commented Feb 25, 2021

ehsannas left a comment

Choose a reason for hiding this comment

jaebaek commented Feb 26, 2021

AppVeyorBot commented Feb 26, 2021

seppala2 commented Jan 26, 2021 •

edited by jaebaek

Loading

ghost commented Jan 26, 2021 •

edited by ghost

Loading

jaebaek left a comment •

edited

Loading

seppala2 Feb 20, 2021 •

edited

Loading

jaebaek commented Feb 25, 2021 •

edited

Loading