[0010] Begin Review Period

[llvm-beanz]

Which document does this relate to?
Proposal 0010: vk::BufferPointer

Review Period

With all outstanding issues resolved, the vk::BufferPointer spec is now entering a final review period. This period is a final call for feedback on the proposal. All feedback is due 9/15. If there is no outstanding feedback at EOD 9/15, the spec will be accepted on 9/18.

If you have feedback please either comment on this issue or file an issue against this repository.

[jeremyong]

FYI I think the proposal link is a bad link? I think this might have been what was intended?

[devshgraphicsprogramming]

I have a need for clarification w.r.t. the Get() method.

Ideally I'd like it to return the result_id so I can properly pipe it through any and all inline SPIR-V as a [vk::ext_reference] or [[vk::ext_result_id]] I declare later on.

Original issue:
microsoft/DirectXShaderCompiler#5640

This is basically so I can use things like atomics and other weird things like OpLoad and OpStore directly.

Also I guess it "could" possibly allow me to fix #57 if there's some way to form a new clean OpPointerType PhysicalStorageBuffer from an OpAccessChain on a OpPointerType PhysicalStorageBuffer, then OpBitcast or OpConvertPtrtoUint to get my uint64_t and then construct a new vk::BufferPointer out of that.

Basically I could wrap all the above into a small-ish utility function addrof([[vk::ext_reference]] T)

[devshgraphicsprogramming]

You have a method to construct a vk::BufferPointer from a uint64_t but no method/function to convert a vk::BufferPointer to a uint64_t.

I understand you wish to prevent/ban pointer arithmetic on a vk::BufferPointer but I wish to make my own pointers and references that have arithmetic AND the operator[], therefore I need ways to go back and forth between uin64_t and vk::BufferPointer. Right now I can't get the uint64_t representation of a vk::BufferPointer.

Another route would be to allow operator[] and pointer arithmetic on vk::BufferPointer

All addressing is implicitly done using the . pointer, or indexing an array in the struct T.

Without a way to do vk::BufferPointer to uint64_t this becomes a mess because.

I don't think SPIR-V Validator will like me doing this

struct block_s
{
   float4 x;
   uint16_t histogram[1];
};

and then reading past the end of the array.

And neither will it like me declaring a vk::BufferPointer<uint16_t[1]> and indexing off the end of that.

I don't think you will want to support the syntax abomination which is vk::BufferPointer<uint16_t[]>.

[devshgraphicsprogramming]

EDIT: Addressed in #86

Dereference Method - The Get() method represents the struct const lvalue reference of the pointer to which it is applied.

But I thought that later on you accepted a PR which allowed pointees to be writable (Appendix D of proposal explicitly talks about assigning to .Get()), so it needs to also have a struct lvalue reference non-const qualified method overload.

[devshgraphicsprogramming]

vk::reinterpret_pointer_cast<T, A> allows casting for all other BufferPointer types. For both casts, DstAlign <= SrcAlign must be true.

We should get a vk::BufferPointer<T,DstAlign> vk::alignment_pointer_cast<DstAlign,T,SrcAlign>(vk::BufferPointer<T,SrcAlign>) free of the DstAlign<=SrcAlign constraint, which would be kinda like a const_cast for the people who claim "I 100% know what I'm doing".

Two new cast operators are introduced. vk::static_pointer_cast<T, A> allows casting any vk::BufferPointer<SrcType, SrcAlign> to vk::BufferPointer<DstType, DstAlign> only if SrcType is a type derived from DstType.

P.S. Cool, so do we get a std::is_base_of or something like that we can use?

[devshgraphicsprogramming]

An attribute vk::aliased_pointer can be attached to a variable, function parameter or a block member of buffer pointer type. It is assumed that the pointee of an object with this attribute can overlap with the pointee of any other object with this attribute.

I'd like some examples of this in code in an Appendix, I also don't understand if I can turn an aliased pointer to a non-aliased, etc.

[Ipotrick]

Was there any mention of coherent memory access and or atomic access to memory via buffer pointers yet?
Both would be required to replave glsls buffer reference capabilities. Would be a shame if we would still need normal buffers just for these specific uses.

[devshgraphicsprogramming]

Was there any mention of coherent memory access and or atomic access to memory via buffer pointers yet? Both would be required to replave glsls buffer reference capabilities. Would be a shame if we would still need normal buffers just for these specific uses.

I'd expect vk::BufferPointer::Get() to produce a reference just how RWStructuredBuffer::operator[] would, and so for the thing to work with InterlockedAdd etc.

For coherent memory access ( and SPV- atomics i.e. float, and other weird ones) you'd need what I'm alluding to here: #84 (comment)

Because with Vulkan Memory Model (and in C++11), you don't really declare a pointer/reference coherent/volatile but the actual memory read/write as needing a memory barrier.

SPV OpLoad and OpStore now take MemoryOperands, and for any memory op inolving a PhysicalStorageBuffer pointer or its access chain you need at least Aligned, you can obviously add MakePointerVisible or MakePointerAvailable for a load or store respectively (obviously in conjuction with NonPrivatePointer).

Mind you this can be all done with current inline SPIR-V without the need for #59 , if only Get() as well as Get().someMember.anotherMember was guaranteed to become a proper [vk::ext_reference]/OpAccessChain.

[llvm-beanz]

I have a need for clarification w.r.t. the Get() method.

Ideally I'd like it to return the result_id so I can properly pipe it through any and all inline SPIR-V as a [vk::ext_reference] or [[vk::ext_result_id]] I declare later on.

I think by result_id you're talking about the SSA value assigned in the ISA. That's not possible nor does it make any sense in the language. Ultimately what you need is for HLSL to support references, adding a bunch of hacks to work around the language not having references will only make it harder for us to get references.

[devshgraphicsprogramming]

I have a need for clarification w.r.t. the Get() method.
Ideally I'd like it to return the result_id so I can properly pipe it through any and all inline SPIR-V as a [vk::ext_reference] or [[vk::ext_result_id]] I declare later on.

I think by result_id you're talking about the SSA value assigned in the ISA. That's not possible nor does it make any sense in the language. Ultimately what you need is for HLSL to support references, adding a bunch of hacks to work around the language not having references will only make it harder for us to get references.

Well at the end of the day I'm thinking of a way to connect a vk::BufferPointer<uint32_t>::Get() to a SPIR-V intrinsic like this

template<typename T>
[[vk::ext_instruction(/* OpLoad */ 61)]]
T special_load([[vk::ext_reference]] T arg, [[vk::ext_literal]] int aligned, , [[vk::ext_literal]] int alignment, [[vk::ext_literal]] int memoryOperand);

template<typename T, uint16_t Alignment>
T volatile_load(inout vk::BufferPointer<T,Alignment> mypointer)
{
   return special_load(mypointer.Get(),/*Aligned*/0x2,Alignment,/*Volatile*/0x1);
}

[devshgraphicsprogramming]

doesn't matter if you give me a pseudo-intrinsic method like

template<typename T, uint16_t Alignment>
vk::result_id<vk::BufferPointer<T,Alignment>::__get_t> accessChain([[vk::ext_reference]] vk::BufferPointer<T,Alignment> ptr); 

which I can then use as a vk::ext_result_id with other intrinsics.

Or whether I can pass the vk::BufferPointer<T,A>::Get() to an intrinsic as the argument where it expects a [[vk::ext_reference]]

[llvm-beanz]

@s-perron, I think I'd like your opinion on how vk::BufferPointer might interact with the Vulkan "inline assembly" stuff since you have a proposal out to revamp all of that.

[Ipotrick]

Would something like GetCoherent() work? Sytntactically that would be clean and its still generic and easy to use.

[s-perron]

I have a need for clarification w.r.t. the Get() method.

Ideally I'd like it to return the result_id so I can properly pipe it through any and all inline SPIR-V as a [vk::ext_reference] or [[vk::ext_result_id]] I declare later on.

Original issue: microsoft/DirectXShaderCompiler#5640

This is basically so I can use things like atomics and other weird things like OpLoad and OpStore directly.

Also I guess it "could" possibly allow me to fix #57 if there's some way to form a new clean OpPointerType PhysicalStorageBuffer from an OpAccessChain on a OpPointerType PhysicalStorageBuffer, then OpBitcast or OpConvertPtrtoUint to get my uint64_t and then construct a new vk::BufferPointer out of that.

Basically I could wrap all the above into a small-ish utility function addrof([[vk::ext_reference]] T)

The result Get() is a reference that can be stored to, so you would be allowed to pass that as a paremeter to a spirv intrinsic that has a [[vk::ext_reference]] as a parameter. The back end will know it needs to pass the address of the memory to the instruction. I do not think anything needs to be changed for the spec to support what you want to do.

[devshgraphicsprogramming]

The result Get() is a reference that can be stored to, so you would be allowed to pass that as a paremeter to a spirv intrinsic that has a [[vk::ext_reference]] as a parameter. The back end will know it needs to pass the address of the memory to the instruction. I do not thing anything needs to be changed for the spec to support what you want to do.

Ok final question @s-perron, what about?

struct MyStruct
{
   float somefloat;
   uint32_t someint;
};

   vk::BufferPointer<MyStruct> mypointer(pushConstants.someuint64t);
   special_load(mypointer.Get().someint,/*Aligned*/0x2,Alignment,/*Volatile*/0x1);

basically will "member access operator" applied to the reference obtained via Get() still work for an intrinsic as a [[vk::ext_reference]] parameter?

[devshgraphicsprogramming]

There seem to be 3 classes/cases of vk::BufferPointer interaction with #59 and [[vk::ext_reference]]:

1. when you pass the result of vk::BufferPointer::Get() invocation directly in the intrinsic function call (this @s-perron already addressed)
2. when you pass the result of vk::BufferPointer::Get().someMember.anotherMember invocation directly in the instrinsic function call (this I need confirmed as per my comment above)
3. NEW: when you pass the vk::BufferPointer itself, either as an l-value or a copy or anything, related comment in [0010] Method to obtain uint64_t from BufferPointer #93 is [0010] Method to obtain uint64_t from BufferPointer #93 (comment)

for 1 and 2 I'm assuming no temporaries, copies, etc. that would require references to work in HLSL-the-language as opposed to the AST (where they must be references for anything to work really).

[Dolkar]

I second that without pointer arithmetic or the ability to have pointers to a dynamically sized array, this feature will be of limited usefulness.

[s-perron]

when you pass the result of vk::BufferPointer::Get().someMember.anotherMember invocation directly in the instrinsic function call (this I need confirmed as per my comment above)

You can pass any lvalue to the spir-v intrinsic parameters annotated with vk::reference_ext. When you take the dot operator on an lvalue, you get another value. That means,

T a;
a.member = 10;

is legal. That means that a.member can be passed as a vk::reference_ext parameter. In spir-v the address of the member will be the operand of the instruction.

[devshgraphicsprogramming]

@s-perron last question, what about case (3) which is slapping the pointer itself into the intrinsic (OpConvertPtrToU and OpConvertUToPtr) that expect an OpPointerType as an operand in place of the vk::reference_ext?

[s-perron]

@s-perron last question, what about case (3) which is slapping the pointer itself into the intrinsic (OpConvertPtrToU and OpConvertUToPtr) that expect an OpPointerType as an operand in place of the vk::ext_reference?

Sorry forgot to get back to you about that one. Given the current spec you should be able to do:

template<class T>
[[vk::ext_instruction(/* OpConvertPtrToU */ 117)]]
uint64_t ConvertAddressToUInt( [[vk::ext_reference]] T obj);

vk::BufferPointer<MyStruct> mypointer(pushConstants.someuint64t);
uint64_t address = ConvertAddressToUInt(mypointer.Get());

The idea is that you want to pass the value of the pointer, which is the address of mypointer.Get(), and not the address of the pointer.

[devshgraphicsprogramming]

@s-perron last question, what about case (3) which is slapping the pointer itself into the intrinsic (OpConvertPtrToU and OpConvertUToPtr) that expect an OpPointerType as an operand in place of the vk::reference_ext?

Sorry forgot to get back to you about that one. Given the current spec you should be able to do:

template<class T>
[[vk::ext_instruction(/* OpConvertPtrToU */ 117)]]
uint64_t ConvertAddressToUInt( [[vk::reference_ext]] T obj);

vk::BufferPointer<MyStruct> mypointer(pushConstants.someuint64t);
uint64_t address = ConvertAddressToUInt(mypointer.Get());

The idea is that you want to pass the value of the pointer, which is the address of mypointer.Get(), and not the address of the pointer.

okay, so this seems like all my usages would be 100% covered.

P.S. Just for completeness, can you think of any case for which I'd need the address of the pointer itself?

[s-perron]

Here is an simple example with a local variable: https://godbolt.org/z/hWYbnavbP. Note that this will fail validation because this convert is illegal. However, with a buffer pointer object it should be legal.

P.S. Just for completeness, can you think of any case for which I'd need the address of the pointer itself?

I can't think of one, but someone probably will. :)

[devshgraphicsprogramming]

I can't think of one, but someone probably will. :)

shouldn't we then define how this can be achieved?

[devshgraphicsprogramming]

@s-perron last question, what about case (3) which is slapping the pointer itself into the intrinsic (OpConvertPtrToU and OpConvertUToPtr) that expect an OpPointerType as an operand in place of the vk::ext_reference?

Sorry forgot to get back to you about that one. Given the current spec you should be able to do:

template<class T>
[[vk::ext_instruction(/* OpConvertPtrToU */ 117)]]
uint64_t ConvertAddressToUInt( [[vk::ext_reference]] T obj);

vk::BufferPointer<MyStruct> mypointer(pushConstants.someuint64t);
uint64_t address = ConvertAddressToUInt(mypointer.Get());

The idea is that you want to pass the value of the pointer, which is the address of mypointer.Get(), and not the address of the pointer.

I'm just thinking out loud, if Get() returns an l-value and Get().someMember also returns as l-value, then we can use ConvertAddressToUint(mypointer.Get().member) as an "address of" function which would close #57 ? Or did I get something horribly wrong?

[s-perron]

I can't think of one, but someone probably will. :)

shouldn't we then define how this can be achieved?

This would not be legal generally.

I would expect ConvertAddressToUInt(mypointer) to work as long as the object lives in the right address space. Things can get tricky if you passed the BufferPointer as a parameter, or created a local copy.

[devshgraphicsprogramming]

I can't think of one, but someone probably will. :)

shouldn't we then define how this can be achieved?

This would not be legal generally.

I would expect ConvertAddressToUInt(mypointer) to work as long as the object lives in the right address space. Things can get tricky if you passed the BufferPointer as a parameter, or created a local copy.

I don't think its desirable for ConvertAddressToUint(mypointer) to work the same as ConvertAddressToUint(mypointer.Get()), I don't think you'd want that to work.

I'm just wondering if there's any SPIR-V opcode where passing the address to the pointer itself would make sense.
IDK, maybe making a copy?

[llvm-beanz]

Taking the address of a pointer in device-local memory is out of scope for this proposal. This proposal explicitly states that all addresses that vk::BufferPointer can refer to are host-local memory, but the pointer would be device-local.

I don't think we should support address-of or other more complex pointer scenarios until we add actual pointers and references to the language.

[devshgraphicsprogramming]

Taking the address of a pointer in device-local memory is out of scope for this proposal. This proposal explicitly states that all addresses that vk::BufferPointer can refer to are host-local memory, but the pointer would be device-local.

I don't think we should support address-of or other more complex pointer scenarios until we add actual pointers and references to the language.

I'm fine with that as long as the cases we discussed with s-perron work.

Howver I'd advise to define what happens when you try to pass a vk::BufferPointer as opposed to the result of Get() to a [vk::ext_reference] to be an error and issue a compiler diagnostic.

[natevm]

Taking the address of a pointer in device-local memory is out of scope for this proposal. This proposal explicitly states that all addresses that vk::BufferPointer can refer to are host-local memory, but the pointer would be device-local.

I don't think we should support address-of or other more complex pointer scenarios until we add actual pointers and references to the language.

Are you confusing host-local and device-local here? I’d rarely if ever want want to fetch data clear across the PCI-E bus into CPU (aka host) memory, but you make this sound like it’s a requirement…

vulkan’s device addresses almost always refer to device local memory…

[natevm]

P.S. Just for completeness, can you think of any case for which I'd need the address of the pointer itself?

I can't think of one, but someone probably will. :)

The obvious application would be a two-dimensional array, with each entry having variable size. For example, an array of buffer pointers to individual mesh indices. First dimension would be the mesh ID, second would be the triangle of that mesh. In a game with many emissive meshes, one might want to trace rays to a random triangle of a random mesh.

Fortunately, if a vk::BufferPointer can be constructed from a uint64_t, I think this is mostly a matter of syntax sugar.

[natevm]

I second that without pointer arithmetic or the ability to have pointers to a dynamically sized array, this feature will be of limited usefulness.

Iiuc, vk::BufferPointer construction from uint64_t would allow pointer arithmetic to be done with one level of indirection.

You start slipping into out-of-spec behavioral issues like with vk::RawBufferLoad/Store though. Still, it seems obvious to me that developers will do exactly this arithmetic on uint64_t, then vk::BufferPointer construction to skirt around the currently imposed limitations.

I’m guessing that issues will arise from that, which will require a subsequent spec proposal involving dereferencing. I can understand why that might deserve its own proposal.

[devshgraphicsprogramming]

You start slipping into out-of-spec behavioral issues like with vk::RawBufferLoad/Store though. Still, it seems obvious to me that developers will do exactly this arithmetic on uint64_t, then vk::BufferPointer construction to skirt around the currently imposed limitations.

as it stands you can't go back from a vk::BufferPointer to a uint64_t, so you'll end up making your own unholy wrappers like me: https://godbolt.org/z/fxzqjddfa

[llvm-beanz]

Taking the address of a pointer in device-local memory is out of scope for this proposal. This proposal explicitly states that all addresses that vk::BufferPointer can refer to are host-local memory, but the pointer would be device-local.
I don't think we should support address-of or other more complex pointer scenarios until we add actual pointers and references to the language.

Are you confusing host-local and device-local here? I’d rarely if ever want want to fetch data clear across the PCI-E bus into CPU (aka host) memory, but you make this sound like it’s a requirement…

vulkan’s device addresses almost always refer to device local memory…

The spec (as written) says “All buffer pointers are presumed to point into the host memory address space. No new address space attributes are proposed.”. If these are intended to be device-local memory, that wording needs to be changed.

[natevm]

The spec (as written) says “All buffer pointers are presumed to point into the host memory address space. No new address space attributes are proposed.”. If these are intended to be device-local memory, that wording needs to be changed.

In my opinion we should change it, unless I’m missing something important. Really, it shouldn’t matter if the Vulkan device address refers to device or host local memory. Indeed, I use both with raw buffer store/load. It just needs to be a valid Vulkan device address.

[s-perron]

The spec (as written) says “All buffer pointers are presumed to point into the host memory address space. No new address space attributes are proposed.”. If these are intended to be device-local memory, that wording needs to be changed.

In my opinion we should change it, unless I’m missing something important. Really, it shouldn’t matter if the Vulkan device address refers to device or host local memory. Indeed, I use both with raw buffer store/load. It just needs to be a valid Vulkan device address.

You're right this should change. It should match the language in the vulkan spec. It is just the device address for a buffer. It does not know or care about anything else. The address are suppose to come from a call to vkGetBufferDeviceAddress on the host and passed to the shader as a parameter. There are no restrictions on the type of the device memory. It could have any of these properties.

[greg-lunarg]

@llvm-beanz All 0010 issues are now closed. Can this feature's review period now be closed?

[llvm-beanz]

The feature has been marked as accepted. Closing as we will file additional issues if any further changes arise.