Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[0011] Resource element type validation #69
[0011] Resource element type validation #69
Changes from 2 commits
7b0a53b
cf382f7
2955655
55b8156
38a0efe
aeedba4
2cd9493
46a67ae
5b9869e
563aa4a
9f8ba5a
2fb070c
b7497e4
a2f38c1
fe417de
6b19731
5c0096c
739fe7d
dd1c1bd
f4a44ae
036cf48
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think you're the sponsor
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 think you can remove this and the PRs line
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.
Is this a new acronym? Not objecting, but I don't think I've seen it before.
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, completely new as far as I know. Just thought I'd use an acronym since that phrase is littered everywhere in this spec.
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.
FWIW, when I see RET I think of this: https://www.felixcloutier.com/x86/ret
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.
bool, uint64_t and double are currently allowed as well https://godbolt.org/z/eb7Yh73W6
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.
If I understand correctly from my conversation with @tex3d, though the
uint64_t
anddouble
cases compile, they will cause undefined behavior at runtime, and we do not want to allow any types that are larger than 32 bits.Even if 2 64-bit types can technically fit in 4 32-bit fields, it's a hard limit that there may not be more than 4 elements, and each element may not be more than 32 bits.
We allow
bool
.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.
Support for 64-bit values seems like a totally reasonable thing to do. Is there something fundamental about them that mean we can't support them properly, or is this just that DXC is buggy?
What about in SPIR-V today? Looks like SPIR-V supports
int64_t2
.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.
It's something fundamental (If I recall correctly, I think it had to do something with the hardware converter / DXGI format being capped at 32 bits in D3D, and that is not changeable).
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 don't think I said anything about them not working or causing undefined behavior. The undefined behavior case was tied to matching unorm or snorm in HLSL with the DXGI resource type for typed UAVs.
They definitely are supported on DXC (by decomposing to 32-bit components), and FXC the same way IIRC (for double).
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.
Perhaps what was thought of was the fact that you can't use these with Sample operations (only Load/Store), since it's equivalent to a typed resource with an int32 component type, which doesn't support those operations.
In fact, if you try it, you get a validation error (we only catch it there), as it attempts to use a 64-bit overload for the sample operation, and define an invalid
%dx.types.ResRet.f64
type.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.
Circling back to this conversation, I recall that this isn't correct anymore - there is more to add.
We added integer sampling support in SM 6.7. This relaxed checks in the front end that disallowed integer types. So integer types are actually allowed in SM 6.7 on (but not for comparison sampling).
Unfortunately, when 64-bit types are used with sample operations, those resource return types won't be translated into two i32 values per 64-bit value. That's what leads to the invalid operation type for these caught by validation. These types could be supported using integer sampling, if we had broken them up as we do for load operations.
I think this means that when using a 64-bit component type in a typed resource element, we should consider this as being implicitly translated to two 32-bit uints per 64-bit component for the actual type used. Then the rules are applied according to the translated element type.
There is one thing bugging me: double component type seemingly should trigger an error in
IsValidObjectElement
, but it doesn't. See the code here:https://github.com/microsoft/DirectXShaderCompiler/blob/a023a95f73618e8ca1791147d3722a3e1ea7608f/tools/clang/lib/Sema/SemaHLSL.cpp#L6058
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.
In addition, vectors and matrices of 4 elements or fewer should be allowed both in and out of structs. I recognize that the matrix support is not part of this proposal, but it might be worth mentioning as part of the introduction potentially with a parenthetical or something that says it will be addressed separately.
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.
Can HLSL authors interact with handle types directly?
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.
It should technically be possible.
I wrote a test in
clang\test\SemaHLSL\resource_binding_attr_error.hlsl
where__hlsl_resource_t
, the handle type, was directly declared, so it is spellable.But I don't think we intend users to interact with them.
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.
Technically the
__hlsl_resource_t
type is spellable, but there is not much one can do with it. It needs to be decorated with resource type attributes to signify what kind of resource it is representing, and it will never get automatic binding unless it is embedded in a struct that looks like a resource class (has the handle as a first field named "h").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.
"allow list" or similar is a preferable term to whitelist.
Any reference to code seems more at home in detailed design. I think the same information can be provided by saying what the current clang compiler will and won't accept.
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.
those types within a vector are also currently supported
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.
Per the link above, bool itself seems to work. Bool vectors do not work in any respect, but that's another matter: https://godbolt.org/z/eb7Yh73W6
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.
Hmmm, there must have been a fix over the past several months for there to no longer be any errors. I'll update this statment.
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 get from this that there will be one set of type_traits for all that will be the most restrictive, so the typed buffer requirements and then the structured buffers will have some kind of exceptions? Is that simple than giving each its own set of type_traits or have I misunderstood?
From the below, it sounds like there will be two sets of type_traits, but this paragraph makes it sound like there will only be one set of type_traits that will be applied to "any RET".
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, what I intend to communicate is that that
RawBuffer
set of type traits will be applied to those resource types on declaration in HLSLExternalSemaSource.cpp, and the non-RawBuffer
type traits will be applied to their respective resource types. I'll tweak the wording.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.
typo
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.
In LLVM parlance, I think "aggregate type" applies to structs and arrays, but not vectors nor matrices. https://github.com/llvm/llvm-project/blob/a1d64626ba16f5128530ac771c6e641b1155184f/llvm/include/llvm/IR/Type.h#L291-L293
So I think we need another term for 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 think there are two criteria here that are munged into one. That's how DXC implies it is working as well and it was only relevant when we got sub 32-bit types, but now that we have, there are two separate requirements here.
The reason for separating these is that 8 uint16s will fit in the size, but is more than 4 components and is rejected by DXC.
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.
To your second point, I think the constraint is slightly stronger, each element may not exceed 32 bits. I alluded to the reason earlier.
Yes, the wording here can be made clearer, I'll adjust.
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.
This would be a bit easier to follow if, instead of arbitrary struct names, they described what they contained. So
struct x
could bestruct oneInt
andstruct a
could bestruct twoInt
etcThere 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.
It also has 4 floats and 2 ints, which is more than 4 components and also won't fit in 16 bytes. Might be better to keep the examples at one violation each.