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

Add SPV_AMDX_shader_enqueue version 2 support #5838

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yavn
Copy link

@yavn yavn commented Oct 4, 2024

@alan-baker
Copy link
Contributor

Could you please rebase on main and update the spirv-headers hash in DEPS to point to at least the merged headers for this extension?

* Update SPIRV-Headers

Co-authored-by: Dan Brown <[email protected]>
Co-authored-by: Maciej Jesionowski <[email protected]>
@yavn yavn force-pushed the SPV_AMDX_shader_enqueue_v2 branch from 6e411f5 to bef6468 Compare October 9, 2024 20:10
@yavn
Copy link
Author

yavn commented Oct 9, 2024

Hi @alan-baker , done and done.

Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall there seem to be a lack of tests for this functionality. Please add some tests for the new validation.

Separately, when reading the spec for this extension you seem to be validating things that are missing from the spec. I'd suggest double checking and updating the extension text to match this validation.

source/val/validate_annotation.cpp Show resolved Hide resolved
source/val/validate_function.cpp Outdated Show resolved Hide resolved
source/val/validate_memory.cpp Show resolved Hide resolved
source/val/validate_mode_setting.cpp Show resolved Hide resolved
source/val/validate_type.cpp Show resolved Hide resolved
source/opt/types.cpp Show resolved Hide resolved
source/opt/types.cpp Show resolved Hide resolved
Comment on lines +971 to +972
case spv::Op::OpDecorate:
case spv::Op::OpDecorateId: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some explanation is need for why this will work. The decorations in the type were written assuming that the extra operands are literals, and not ids. By doing this you are losing this distinction because the opcode is not part of the "data".

Some comment might be needed to explained how people looking at the decorations in the type can know how to interpret the operands. Can this be done based solely on the decoration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SPIR-V spec tags ids vs. literals on the decorations themselves, and then says anything with an ID has to go through OpDecorateId. I think it's just that up until now we've not had any decorations on types that used IDs.

Are you looking for code comments here? Or some explanation beyond that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is a theoretical bug. However, I could be wrong if there is a detail in the spec I don't know about.

The decoration is stored as a vector of integers. This is currently interpreted as the first element is the decoration id followed by a series of immediate values. This this change that is no longer always true. It could be a decoration id followed by a series of ids. How can people reading the decorations associated with the type know the difference?

A potential problem. Suppose there are two types that are the exact same except that one has a decoration represented using OpDecorate and another with OpDecorateId:

OpDecorate %T1 SomeDecoration 100
OpDecorateId %T2 SomeDecoration %100

When hashing %T1 and %T2, their decorations will have the exact same integers: {SomeDecoration, 100}. They will then hash to the same value, and be considered the same type, which they are not.

I could see there being a convention, which I do not see in the spec, that a decoration is either always used with OpDecorate or OpDecorateId, so this will not happen. If that is the case, we will need a authoritative list of which decorations use one opcode and which one use another. That way code that accesses Type::decorations() will know how to interpret them. Or we could come up with another way of disinguishing them by adding the opcode to the list or adding a separate list like we do for member variables. I'm fine with either solution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think any decoration can be used with both IDs and literals. Usually we have a separate enum for those cases (e.g. LocalSize and LocalSizeID).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way in Spirv-tools to be able to tell the difference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are expecting people to look at the decoration to know if it came from opdecorateid or not, then a comment should be added to the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do attempt to validate this in the validator. That requires keeping it up-to-date, but hopefully that would happen.

Otherwise you'd have to parse the grammar and check the operands which is awkward.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s-perron it's not clear to me what commentary you're looking for here. If this was the validator I agree it would need something explaining "validated elsewhere", but for the optimiser we should be assuming valid code, and it's plain at least to me that decorating with either instruction would achieve the same goal.

The decoration itself can be separately decoded later in isolation of the instruction it was provided with, because the decoration decides what its operands mean, not the instruction. You never need to know which instruction it came with, you can just assume was specified with the right one.

Are you looking for a comment outlining something like that?

source/opt/types.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants