-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[DirectX] add enum for PSV resource type/kind/flag. #106227
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,11 @@ Parts: | |
MaximumWaveLaneCount: 4294967295 | ||
ResourceStride: 16 | ||
Resources: | ||
- Type: 1 | ||
- Type: Sampler | ||
Space: 2 | ||
LowerBound: 3 | ||
UpperBound: 4 | ||
- Type: 128 | ||
- Type: Invalid | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if this is a dumb question, but I'm not very familiar with the YAML printer. Is this printing the enum value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding is that for enum class types, you cannot say 128 anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just confused because I see some new definitions and I see test changes, but I don't see the producer of the value changing anywhere so it isn't clear to me what the actual difference in behaviour is. I'm guessing the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The value should not change, unless it does not have a mapping to the enum values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems kind of unfortunate to not see the actual value that's in the structure in our debug tools - couldn't this mean that we end up putting garbage values in the actual container without noticing it in our tests? I guess if that's how the yaml printing works it's a wider issue and doesn't need to block this change in particular. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
||
Space: 32768 | ||
LowerBound: 8388608 | ||
UpperBound: 2147483648 | ||
|
@@ -48,11 +48,11 @@ Parts: | |
# CHECK-NEXT: MaximumWaveLaneCount: 4294967295 | ||
# CHECK-NEXT: ResourceStride: 16 | ||
# CHECK-NEXT: Resources: | ||
# CHECK-NEXT: - Type: 1 | ||
# CHECK-NEXT: - Type: Sampler | ||
# CHECK-NEXT: Space: 2 | ||
# CHECK-NEXT: LowerBound: 3 | ||
# CHECK-NEXT: UpperBound: 4 | ||
# CHECK-NEXT: - Type: 128 | ||
# CHECK-NEXT: - Type: Invalid | ||
# CHECK-NEXT: Space: 32768 | ||
# CHECK-NEXT: LowerBound: 8388608 | ||
# CHECK-NEXT: UpperBound: 2147483648 | ||
|
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.
Nit: Style-wise, we usually put the
#define
immediately before#include
of a def file, like so:I find this makes it more obvious that the two things are connected. This suggestion also applies to the enum definitions in the header.
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 was about to give similar feedback in that llvm/include/llvm/BinaryFormat/DXContainer.h but it pretty consistently puts the
#define
before the enum.I wonder if a fix here, or maybe in a follow up, to update DXContainer to this way would be worth doing?