[SPIRV] Implements vk::BufferPointer proposal #7163

danbrown-amd · 2025-02-27T00:16:52Z

Implements vk::BufferPointer proposal.
Closes #6489.

github-actions · 2025-02-28T01:18:17Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Keenuts

I need to look more, early feedback

Keenuts · 2025-02-28T12:49:14Z

tools/clang/include/clang/AST/HlslTypes.h

+/// <summary>Adds a constructor declaration to the specified class
+/// record.</summary> <param name="context">ASTContext that owns
+/// declarations.</param> <param name="recordDecl">Record declaration in which
+/// to add constructor.</param> <param name="resultType">Result type for
+/// constructor.</param> <param name="paramTypes">Types for constructor
+/// parameters.</param> <param name="paramNames">Names for constructor
+/// parameters.</param> <param name="declarationName">Name for
+/// constructor.</param> <param name="isConst">Whether the constructor is a
+/// const function.</param> <returns>The method declaration for the
+/// constructor.</returns>


nit:
Is doxygen still generatable? (Couldn't make it work).
If no, might be more readable to have a normal comment.

I don't know, but I did it that way for consistency with the rest of the file. Let me know if you still want it changed.

tools/clang/lib/AST/HlslTypes.cpp

tools/clang/lib/SPIRV/CapabilityVisitor.cpp

tools/clang/lib/Sema/SemaHLSL.cpp

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.read.hlsl

s-perron

This generally looks go. Thanks for getting this done. It is seems solidly implemented. I have a few concerns about the potential for future errors. With a few asserts we could address that.

I have not looked at the tests yet, nor have I tried running anything yet. I'll try to tomorrow.

tools/clang/include/clang/AST/OperationKinds.h

tools/clang/lib/AST/HlslTypes.cpp

tools/clang/lib/SPIRV/EmitVisitor.cpp

tools/clang/lib/SPIRV/SpirvContext.cpp

tools/clang/lib/SPIRV/CapabilityVisitor.cpp

tools/clang/lib/SPIRV/SpirvEmitter.cpp

s-perron

I have a suggestion for a a few extra tests. Once they are in, I feel comfortable approving.

Can you also add a tests where an atomic operation is done? We want to be sure that works too.

Is the has_feature uses anywhere? We should make sure that works.

tools/clang/lib/Lex/PPMacroExpansion.cpp

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.linked-list.hlsl

Keenuts

Thanks for the work!
Good improvements, but looks like there are still some failure cases:

Seems like I also reach an assert when this snippet:

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  float a;
};

typedef vk::BufferPointer<Content> BufferContent;
typedef vk::BufferPointer<BufferContent> BufferBufferContent;

[[vk::push_constant]]
BufferContent buffer;

[numthreads(1, 1, 1)]
void main() {
  float tmp = buffer.Get().a;
  buffer.Get().a = tmp;
}

tools/clang/lib/SPIRV/SpirvEmitter.cpp

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.read.hlsl

Keenuts · 2025-03-18T10:23:35Z

Also get an assert/validation errors with those 2 snippets:

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  uint a;
};

typedef vk::BufferPointer<uint> BufferContent;

[[vk::push_constant]]
BufferContent buffer;

[numthreads(1, 1, 1)]
void main() {
  uint data = buffer.Get();
  buffer.Get() = data;
}

Error: assert(IsPtr(ptr_id) && "Cannot get the variable when input is not a pointer.")
(assert in SPIRV-Tools)

Looks like removing optimizations or changing the snippet to this shows the validation error instead:

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  uint a;
};

typedef vk::BufferPointer<uint> BufferContent;

[[vk::push_constant]]
BufferContent buffer;

[numthreads(1, 1, 1)]
void main() {
  buffer.Get() = 1;
}

Keenuts · 2025-03-18T10:30:02Z

Found 2 other case:

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  int a;
};

typedef vk::BufferPointer<Content> BufferContent;
typedef vk::BufferPointer<BufferContent> BufferBuffer;

//[[vk::push_constant]]
//BufferContent buffer;

RWStructuredBuffer<BufferBuffer> rwbuf;

void foo(BufferContent bc) {
  bc.Get().a = 1;
}

[numthreads(1, 1, 1)]
void main() {
  foo(rwbuf[0].Get());
}

fatal error: generated SPIR-V is invalid: [VUID-StandaloneSpirv-PhysicalStorageBuffer64-04708] Memory accesses with PhysicalStorageBuffer must use Aligned.
  %21 = OpLoad %_ptr_PhysicalStorageBuffer_Content %20

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  int a;
};

typedef vk::BufferPointer<Content> BufferContent;
typedef vk::BufferPointer<BufferContent> BufferBuffer;

//[[vk::push_constant]]
//BufferContent buffer;

RWStructuredBuffer<BufferBuffer> rwbuf;

// Wrong type in the parameter.
void foo(BufferContent bc) {
  bc.Get().a = 1;
}

[numthreads(1, 1, 1)]
void main() {
  foo(rwbuf[0]);
}

fatal error: generated SPIR-V is invalid: OpAccessChain reached non-composite type while indexes still remain to be traversed.
  %21 = OpAccessChain %_ptr_PhysicalStorageBuffer_int %20 %int_0

Keenuts · 2025-03-18T10:38:06Z

Looks like casting would also need additional restrictions:

// RUN: %dxc -E main -T cs_6_7  %s -spirv -E main | FileCheck %s

struct Content {
  int a;
};

typedef vk::BufferPointer<Content> BufferContent;
typedef vk::BufferPointer<BufferContent> BufferBuffer;

RWStructuredBuffer<BufferContent> buf;

void foo(const BufferContent bc) {
  bc.Get().a = 1;
}

[numthreads(1, 1, 1)]
void main() {
  static BufferContent bcs = buf[0];
  static BufferBuffer bbs = (BufferBuffer)bcs;
}

fatal error: generated SPIR-V is invalid: Expected unsigned int scalar type as Result Type: ConvertPtrToU
  %19 = OpConvertPtrToU %_ptr_PhysicalStorageBuffer__ptr_PhysicalStorageBuffer_Content %18

danbrown-amd · 2025-03-18T21:25:35Z

Should it actually be legal to have buffer pointer push constants? The front end rejects non-struct push constant declarations., and although values of type vk::BufferPointer are objects, they don't have data members and thus aren't treated as records.

Keenuts · 2025-03-19T12:48:04Z

Should it actually be legal to have buffer pointer push constants?

Mmmm, I don't think there is something that forbids it since we allow it for ConstantBuffers (at least we have one test checking this vk.push-constant.constantbuffer.assign.hlsl)
(@s-perron in case I'm saying something wrong)

What frontend are you referring to?

s-perron · 2025-03-19T13:06:44Z

Should it actually be legal to have buffer pointer push constants?

Mmmm, I don't think there is something that forbids it since we allow it for ConstantBuffers (at least we have one test checking this vk.push-constant.constantbuffer.assign.hlsl) (@s-perron in case I'm saying something wrong)

What frontend are you referring to?

I believe the issue is that Vulkan requires that the type of a push constant must be a struct. That is enforced by the compiler: https://godbolt.org/z/8r38xss5o. Since the vk::BufferPointer is a pointer and not a struct, we should not allow it as a push constant unless it is contained in another struct.

Keenuts · 2025-03-19T13:15:55Z

Ah that's right,
And the ConstantBuffer<MyStruct> is allowed only because it's a struct it contains, but ConstantBuffer<int> is not allowed.

danbrown-amd · 2025-03-25T00:25:48Z

I've ruled out buffer pointers as push constants, tightened up the typing/casting rules, and fixed the missing load alignment bug.

Keenuts

Hello! Thanks for the changes!
I'm fine moving forward with this PR so we can find more bugs with the first users, but I'd like for the fixed bugs to have test cases.
(I provided snippets/run lines, so making them into tests is mostly adding a 2>&1 | FileCheck and CHECK: <expected-error>)

danbrown-amd · 2025-03-27T03:44:11Z

I've pushed the requested tests.

I've rebased on the most recent main and fixed the conflicts locally, but a recent (March 4) change to SPIRV-Tools causes non-termination on the linked list example. It appears that Handler_OpTypePointer_StorageBuffer16BitAccess() in trim_capabilities_pass.cpp doesn't account for recursive types.

When a type was recursive (or for the matter any instruction ID usage), the DFSWhile implementation would loop indefinitely. Added a history map to keep track of the visited instructions. Required for microsoft/DirectXShaderCompiler#7163 Signed-off-by: Nathan Gauër <brioche@google.com>

Keenuts · 2025-03-27T12:58:39Z

Handler_OpTypePointer_StorageBuffer16BitAccess

Ah right, thanks for finding, sent KhronosGroup/SPIRV-Tools#6070 to fix the issue.

When a type was recursive (or for the matter any instruction ID usage), the DFSWhile implementation would loop indefinitely. Added a history map to keep track of the visited instructions. Required for microsoft/DirectXShaderCompiler#7163 Signed-off-by: Nathan Gauër <brioche@google.com>

Keenuts · 2025-03-27T17:33:05Z

Once #7269 is merged you'll be able to rebase 👍️

(https://github.com/microsoft/hlsl-specs/blob/main/proposals/0010-vk-buffer-ref.md) Addresses issue microsoft#6489.

amaiorano · 2025-04-03T17:35:40Z

tools/clang/lib/Sema/SemaHLSL.cpp

@@ -1232,6 +1237,9 @@ static const ArBasicKind g_AnyOutputRecordCT[] = {
 static const ArBasicKind g_DxHitObjectCT[] = {AR_OBJECT_HIT_OBJECT,
                                              AR_BASIC_UNKNOWN};

+static const ArBasicKind g_VKBufferPointerCT[] = {AR_OBJECT_VK_BUFFER_POINTER,


We're getting build breaks in Chromium where ENABLE_SPIRV_CODEGEN is not defined. I think this decl should be wrapped in #ifdef ENABLE_SPIRV_CODEGEN?

I'm working on a patch.

I'm testing a fix now.

Created pull request.

PR microsoft#7163 added support for vk::BufferPointer It did not build when building for non-SPIR-V scenarios. Bug: crbug.com/408215487

([SPIRV] Implements vk::BufferPointer proposal)

…fferPointer proposal) (#7306) #ifdef ENABLE_SPIRV_CODEGEN was omitted in several places.

Modification copyright notices were added in error to files changed in [PR #7163](#7163).

danbrown-amd requested a review from a team as a code owner February 27, 2025 00:16

github-project-automation bot added this to HLSL Roadmap Feb 27, 2025

github-project-automation bot moved this to New in HLSL Roadmap Feb 27, 2025

danbrown-amd force-pushed the vk-buffer-pointer-pr branch from 76c4743 to 0d60681 Compare February 28, 2025 01:17

s-perron requested review from Keenuts and s-perron February 28, 2025 01:22

danbrown-amd force-pushed the vk-buffer-pointer-pr branch from 0d60681 to 78eb760 Compare February 28, 2025 01:41

Keenuts reviewed Feb 28, 2025

View reviewed changes

s-perron reviewed Mar 3, 2025

View reviewed changes

danbrown-amd force-pushed the vk-buffer-pointer-pr branch from 75cd0be to 2354f87 Compare March 5, 2025 04:54

s-perron reviewed Mar 5, 2025

View reviewed changes

tools/clang/lib/Lex/PPMacroExpansion.cpp Show resolved Hide resolved

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.linked-list.hlsl Show resolved Hide resolved

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.linked-list.hlsl Outdated Show resolved Hide resolved

s-perron previously approved these changes Mar 17, 2025

View reviewed changes

Keenuts reviewed Mar 18, 2025

View reviewed changes

tools/clang/lib/SPIRV/SpirvEmitter.cpp Outdated Show resolved Hide resolved

tools/clang/test/CodeGenSPIRV/vk.buffer-pointer.read.hlsl Show resolved Hide resolved

Keenuts reviewed Mar 25, 2025

View reviewed changes

Keenuts mentioned this pull request Mar 27, 2025

[OPT] fix loop with recursive types in trim pass KhronosGroup/SPIRV-Tools#6070

Merged

[spirv] Implements vk::BufferPointer proposal

0322faf

(https://github.com/microsoft/hlsl-specs/blob/main/proposals/0010-vk-buffer-ref.md) Addresses issue microsoft#6489.

danbrown-amd force-pushed the vk-buffer-pointer-pr branch from 9581da6 to 0322faf Compare March 28, 2025 00:14

danbrown-amd requested a review from Keenuts March 28, 2025 16:04

danbrown-amd dismissed s-perron’s stale review via 0322faf April 1, 2025 01:05

Keenuts approved these changes Apr 1, 2025

View reviewed changes

Keenuts requested a review from s-perron April 1, 2025 16:27

s-perron approved these changes Apr 1, 2025

View reviewed changes

s-perron enabled auto-merge (squash) April 1, 2025 21:26

s-perron merged commit 9eb7119 into microsoft:main Apr 2, 2025
12 checks passed

github-project-automation bot moved this from New to Done in HLSL Roadmap Apr 2, 2025

amaiorano reviewed Apr 3, 2025

View reviewed changes

dneto0 added a commit to dneto0/DirectXShaderCompiler that referenced this pull request Apr 3, 2025

Fix vk::BufferPointer for non-SPIR-V builds

2b9fa98

PR microsoft#7163 added support for vk::BufferPointer It did not build when building for non-SPIR-V scenarios. Bug: crbug.com/408215487

dneto0 added a commit to dneto0/DirectXShaderCompiler that referenced this pull request Apr 3, 2025

Fix vk::BufferPointer for non-SPIR-V builds

d4cd822

PR microsoft#7163 added support for vk::BufferPointer It did not build when building for non-SPIR-V scenarios. Bug: crbug.com/408215487

dneto0 mentioned this pull request Apr 3, 2025

Fix vk::BufferPointer for non-SPIR-V builds #7305

Closed

danbrown-amd added a commit to danbrown-amd/DirectXShaderCompiler that referenced this pull request Apr 3, 2025

Fixes non-SPIR-V build, broken by PR microsoft#7163

f710e6c

([SPIRV] Implements vk::BufferPointer proposal)

s-perron pushed a commit that referenced this pull request Apr 3, 2025

Fixes non-SPIR-V build, broken by PR #7163 ([SPIRV] Implements vk::Bu…

85f3432

…fferPointer proposal) (#7306) #ifdef ENABLE_SPIRV_CODEGEN was omitted in several places.

danbrown-amd mentioned this pull request May 22, 2025

Removes improper modification copyright notice. #7477

Merged

pow2clk pushed a commit that referenced this pull request May 22, 2025

Removes improper modification copyright notice. (#7477)

d72e2b1

Modification copyright notices were added in error to files changed in [PR #7163](#7163).

[SPIRV] Implements vk::BufferPointer proposal #7163

[SPIRV] Implements vk::BufferPointer proposal #7163

Uh oh!

Conversation

danbrown-amd commented Feb 27, 2025

Uh oh!

github-actions bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keenuts left a comment

Choose a reason for hiding this comment

Uh oh!

Keenuts Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

danbrown-amd Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Keenuts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Keenuts commented Mar 18, 2025

Uh oh!

Keenuts commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keenuts commented Mar 18, 2025

Uh oh!

danbrown-amd commented Mar 18, 2025

Uh oh!

Keenuts commented Mar 19, 2025

Uh oh!

s-perron commented Mar 19, 2025

Uh oh!

Keenuts commented Mar 19, 2025

Uh oh!

danbrown-amd commented Mar 25, 2025

Uh oh!

Keenuts left a comment

Choose a reason for hiding this comment

Uh oh!

danbrown-amd commented Mar 27, 2025

Uh oh!

Keenuts commented Mar 27, 2025

Uh oh!

Keenuts commented Mar 27, 2025

Uh oh!

Uh oh!

amaiorano Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

dneto0 Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

danbrown-amd Apr 3, 2025

github-actions bot commented Feb 28, 2025 •

edited

Loading

Keenuts commented Mar 18, 2025 •

edited

Loading