[SPIRV] Implements vk::BufferPointer proposal

[danbrown-amd]

Implements vk::BufferPointer proposal.
Closes #6489.

[github-actions]

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

[Keenuts]

I need to look more, early feedback

[Keenuts]

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

[danbrown-amd]

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.

[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.

[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.

[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;
}

[Keenuts]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[Keenuts]

Handler_OpTypePointer_StorageBuffer16BitAccess

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

[Keenuts]

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

[amaiorano]

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?

[dneto0]

I'm working on a patch.

[danbrown-amd]

I'm testing a fix now.

[danbrown-amd]

Created pull request.