-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add type punning rules to vk::BufferPointer #55
Conversation
Is there currently already a PR that implements these functions? Or is it being worked on? 👀 |
There is a PR implementing something similar, but it was rejected because it did not follow the process being followed here. An implementation of this proposal will likely be based on the previous PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
proposals/0010-vk-buffer-ref.md
Outdated
@@ -163,6 +163,10 @@ A vk::BufferPointer can otherwise be used whereever the HLSL spec does not other | |||
|
|||
Applying HLSL semantic annotations to objects of type vk::BufferPointer is disallowed. | |||
|
|||
### Buffer Pointers and Type Punning Through Unions | |||
|
|||
While buffer pointer types are allowed in unions, type punning with buffer pointer types is disallowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this wording is sufficient.
Take this case:
Buffer<Int> I;
union Pain {
vk::BufferPointer<SomeStruct> Ptr;
uint64_t Int;
double FP;
};
void Init(inout Pain P) {
P.Int = I[0];
}
SomeStruct Load(Pain P) {
return P.Ptr.Get();
}
void Fn() {
Pain P;
Init(P);
SomeStruct S = Load(P);
}
The union P
in Fn
is initialized in Init
to a uint64_t
), then used as a vk::BufferPointer
. What is the expected behavior here?
It can't be a compile error because Init
, Load
and Fn
don't need to be defined in the same translation unit so we can't analyze that usage pattern. Is it undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my intention is that this would be undefined behavior.
In general, type punning across members of a union is considered undefined behavior in C++. I don't see anything in the HLSL union proposal that changes this. So it seems that this should be true no matter the types of the members, including buffer pointers.
I will augment the text of this section to be more explicit about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to call this good enough. I don't think this is the right language or describes the behavior we want, but I think we can defer getting it correct until the Unions proposal actually defines how type punning works.
This adds a resource chapter that documents typed and raw buffers including their methods and compatible operators along with short descriptions of their functionality. It also adds a description of binding annotations for all types. Stub sections are included for constant buffers and samplers, but no information is provided as yet. Fixes microsoft#55 Fixes microsoft#56
Fixes #43