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

update rspirv 0.11 to 0.12 #1118

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

Conversation

Firestar99
Copy link

@Firestar99 Firestar99 commented Jan 23, 2024

The rspirv update is required to get the spirv symbols required for mesh shaders, which will be added in a separate PR.
Original mesh shader PR: #1019

@Firestar99 Firestar99 requested a review from eddyb as a code owner January 23, 2024 18:44
@Firestar99 Firestar99 force-pushed the mesh-ext-shaders branch 5 times, most recently from 5dfcd5c to 2a3dbcb Compare January 23, 2024 21:21
@Firestar99 Firestar99 marked this pull request as draft January 23, 2024 21:26
@Firestar99
Copy link
Author

While porting, I've noticed that rustc_codegen_spirv::spirv_type_constraints::instruction_signatures(Op) somehow seems to validate a lot of SPIR-V Ops on their input and output types, and that spirv/grammar/autogen_table.rs seems to also contain a giant List of auto-generated instructions with all their inputs and outputs. Maybe something could be simplified there?

@Firestar99 Firestar99 changed the title WIP: update rspirv 0.11 to 0.12 update rspirv 0.11 to 0.12 Jan 25, 2024
@Firestar99
Copy link
Author

Basically ready to merge, apart from LiteralFloat still being a todo!. If it's essentially unused, should it just be replaced with some kind of error message, and done with it?

Copy link
Contributor

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Ahh, I see you got around to fixing up the BaryCoord stuff before I did, nice!

Leaving this review more as a note that I will check the subtler aspects in more depth (so we don't accidentally merge too soon), but what I've seen looks great, thank you so much for working on this!

Comment on lines +1361 to +1420
(OperandKind::FPDenormMode, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::FPDenormMode(x)),
Err(()) => self.err(format!("unknown FPDenormMode {word}")),
},
(OperandKind::QuantizationModes, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::QuantizationModes(x)),
Err(()) => self.err(format!("unknown QuantizationModes {word}")),
},
(OperandKind::FPOperationMode, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::FPOperationMode(x)),
Err(()) => self.err(format!("unknown FPOperationMode {word}")),
},
(OperandKind::OverflowModes, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::OverflowModes(x)),
Err(()) => self.err(format!("unknown OverflowModes {word}")),
},
(OperandKind::PackedVectorFormat, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::PackedVectorFormat(x)),
Err(()) => self.err(format!("unknown PackedVectorFormat {word}")),
},
(OperandKind::HostAccessQualifier, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::HostAccessQualifier(x)),
Err(()) => self.err(format!("unknown HostAccessQualifier {word}")),
},
(OperandKind::CooperativeMatrixOperands, Some(word)) => {
match parse_bitflags_operand(COOPERATIVE_MATRIX_OPERANDS, word) {
Some(x) => inst
.operands
.push(dr::Operand::CooperativeMatrixOperands(x)),
None => self.err(format!("Unknown CooperativeMatrixOperands {word}")),
}
}
(OperandKind::CooperativeMatrixLayout, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::CooperativeMatrixLayout(x)),
Err(()) => self.err(format!("unknown CooperativeMatrixLayout {word}")),
},
(OperandKind::CooperativeMatrixUse, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::CooperativeMatrixUse(x)),
Err(()) => self.err(format!("unknown CooperativeMatrixUse {word}")),
},
(OperandKind::InitializationModeQualifier, Some(word)) => match word.parse() {
Ok(x) => inst
.operands
.push(dr::Operand::InitializationModeQualifier(x)),
Err(()) => self.err(format!("unknown InitializationModeQualifier {word}")),
},
(OperandKind::LoadCacheControl, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::LoadCacheControl(x)),
Err(()) => self.err(format!("unknown LoadCacheControl {word}")),
},
(OperandKind::StoreCacheControl, Some(word)) => match word.parse() {
Ok(x) => inst.operands.push(dr::Operand::StoreCacheControl(x)),
Err(()) => self.err(format!("unknown StoreCacheControl {word}")),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow I didn't realize this was so manual. Longer-term might be worth switching to SPIR-T's view of OperandKind, which handles immediate operands in a much more dynamic way, and contains a lot of the same information that .parse() uses in these match arms (but then I'm not sure you could create the typed rspirv Operand value out of that).

@eddyb

This comment was marked as resolved.

@eddyb
Copy link
Contributor

eddyb commented Feb 7, 2024

@Firestar99 #1127 just landed, you'll need to rebase (but I should be able to review concurrently).

@Firestar99 Firestar99 force-pushed the mesh-ext-shaders branch 3 times, most recently from cd0cbf6 to 9bea24f Compare February 7, 2024 09:46
@Firestar99 Firestar99 marked this pull request as ready for review February 7, 2024 10:51
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.

2 participants