Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[0010] Method to obtain uint64_t from BufferPointer #93

Closed
devshgraphicsprogramming opened this issue Sep 7, 2023 · 7 comments · Fixed by #134
Closed

[0010] Method to obtain uint64_t from BufferPointer #93

devshgraphicsprogramming opened this issue Sep 7, 2023 · 7 comments · Fixed by #134
Assignees
Labels
active proposal Issues relating to active proposals

Comments

@devshgraphicsprogramming

Which proposal does this relate to?

0010 - Buffer Pointers

Describe the issue or outstanding question.

In my opinion we need a function/method to convert vk::BufferPointer<T,A> to uint64_t.

This is because the proposal explicitly disallows arithmetic operators on BufferPointer and doesn't mention there being operator[].

Furthermore indexing past the end of an array in a struct is Invalid/UB in SPIR-V, and OpTypeRuntimeArray cannot be slapped onto any odd struct member outside of an StorageBuffer block.
KhronosGroup/SPIRV-Tools#1458

This means that in order to implement the following design pattern from C/C++

struct DiscreteCumulativeProbability
{
   uint count;
   // actual array length determined by `count`
   float values[1];
};

we cannot simply declare in HLSL202x

struct DiscreteCumulativeProbability
{
   uint count;
   // actual array length determined by `count`
   float values[];
};

Even if I didn't need to work around #57 and could get the address of ptr.Get().value[1] without helper structs/code, I'd still need to be able to perform some form of arithmetic/indexing.

Also I guess vk::BufferPointer<T[]> is not the syntax you want to deal with.

Finally being able to just get the uint64_t address value is a matter of control, we currently have this level of control with RawBufferLoad and RawBufferStore and its not nice for it to go away.

Additional context

See this comment: #84 (comment)

@devshgraphicsprogramming devshgraphicsprogramming added the active proposal Issues relating to active proposals label Sep 7, 2023
@devshgraphicsprogramming devshgraphicsprogramming changed the title [0010] [0010] Method to obtain uint64_t from BufferPointer Sep 7, 2023
@devshgraphicsprogramming
Copy link
Author

btw in case I wasn't clear, this is not about having any implicit casts.

I'd find reintepret_cast<uint64_t>(ptr) acceptable, as well as explicit uint64_t() construction, or just any method added to vk::BufferPointer acceptable.

@devshgraphicsprogramming
Copy link
Author

Another acceptable solution would be for the SPIR-V intrinsics to interact with vk::BufferPointer such that the pointer result-id is passed directly to an intrinsic which requires a [[vk::ext_reference]], so I can implement my own cast

template<typename T, uint16_t A>
[[vk::ext_capability(/*PhysicalStorageBufferAddresses*/ 5347)]]
[[vk::instruction(/*OpConvertPtrToU*/117)]]
uint64_t uint64_t_cast([[vk::ext_reference]] vk::BufferPointer<T,A> p);

@devshgraphicsprogramming
Copy link
Author

devshgraphicsprogramming commented Sep 7, 2023

Hmm in order to keep your aliasing analysis intact, I think my suggestion would be to add the following to an aliased_pointer and aliased pointer only:

  • pointer arithmetic (but no compound arithmetic+assign)
  • operator[]
  • conversion to uint64_t

and keep everything "as is" for restrict pointers.

  • Optional Bonus Round: constxepr arithmetic operator overloads to narrow the A on the return type when the operand is constexpr and would cause an a value aligned to A to change its alignment (+/- a value not aligned to A, >> would decrease alignment until 1, << would increase alignment)

And frankly I think vk::aliased_pointer shouldn't be an attribute, it should be baked into the type to disable conversions from restrict to aliased.

@Ipotrick
Copy link

Ipotrick commented Sep 8, 2023

I believe it is important for aliased pointers to allow for ptr arithmetic. I agree with devsh here.

As aliased pointers have sound semantics for ptr arithmetic, i dont see why it wouldnt be supoorted for them, that would be quite sad that was missing.

I also think aliased pointers should be the default as they generally would then allow for more convenient an generalized use, hence be a better default.

In other languages restrict is also not the default but opt in, i believe it should be the same for hlsl.

I also agree with devsh on the alias beeing part of the type.

@s-perron
Copy link
Collaborator

As stated #84 (comment), you can create your own cast to uint64_t using inline spir-v. Can this issue be closed now?

@devshgraphicsprogramming
Copy link
Author

As stated #84 (comment), you can create your own cast to uint64_t using inline spir-v. Can this issue be closed now?

Doing that will probably mess up the compilers "leak analysis" and produce weird issues if we don't cast aliased pointers to uint64_t.

In principle we could close this issue, but a new one should be opened about whether aliase should be the default and restrict be the opt-in.

@greg-lunarg
Copy link
Contributor

@devshgraphicsprogramming I am sympathetic with the desire for a much richer feature that I am proposing here. But my charter is to design something with a more limited capability. By definition it will not meet everyone's needs.

I am fairly certain vk::RawBufferLoad and vk::RawBufferStore are not going away, so this proposal will not take capability away.

So I think this can be closed.

As for this comment:

... but a new one should be opened about whether aliased should be the default and restrict be the opt-in.

@llvm-beanz has endorsed restrict as the default, noting it is most consistent with HLSL. Also, as a Vulkan feature, restrict default is consistent with SPIR-V rules.

@llvm-beanz llvm-beanz moved this to Backlog in HLSL Language Features Nov 8, 2023
s-perron added a commit to s-perron/hlsl-specs that referenced this issue Dec 1, 2023
We have decided to allow casting to uint64_t. It will make create
more opportunities for developers to make mistakes, but they do not
have to use it. The implementation cost does not seem too high.

Fixes microsoft#93
s-perron added a commit to s-perron/hlsl-specs that referenced this issue Dec 4, 2023
We have decided to allow casting to uint64_t. It will make create
more opportunities for developers to make mistakes, but they do not
have to use it. The implementation cost does not seem too high.

Fixes microsoft#93
s-perron added a commit to s-perron/hlsl-specs that referenced this issue Dec 7, 2023
We have decided to allow casting to uint64_t. It will make create
more opportunities for developers to make mistakes, but they do not
have to use it. The implementation cost does not seem too high.

Fixes microsoft#93
s-perron added a commit that referenced this issue Dec 8, 2023
* [0010] Allow casting to uint64_t

We have decided to allow casting to uint64_t. It will make create
more opportunities for developers to make mistakes, but they do not
have to use it. The implementation cost does not seem too high.

Fixes #93

* Remove cast to bool

As Greg suggested, the cast to bool is no longer needed if we can cast
to an int, so we removed it. This commit also adds the uint64_t cast to
the pseudo class definition for BufferPointers.

* Fix linked list example after rebaseing.
@github-project-automation github-project-automation bot moved this from Backlog to Done in HLSL Language Features Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
active proposal Issues relating to active proposals
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants