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

Implement partial OpLine support #193

Merged
merged 3 commits into from
May 5, 2021
Merged

Conversation

khyperia
Copy link
Collaborator

@khyperia khyperia commented May 5, 2021

This implements partial OpLine support. "Partial" because this only supports OpLine in two places: in types_global_values, and inside basic blocks. In reality, OpLine is allowed in types_global_values, inside basic blocks, but also between basic blocks, in the OpFunctionParameter header, between functions, after all functions, and all sorts of other wacky locations. Supporting all those wacky locations would take a fairly significant refactor (for example, adding a Vec<Instruction> in basic blocks for "OpLine/OpNoLines preceding the OpLabel instruction" - because yes, there can be multiple, if we want to faithfully represent all valid spir-v modules)

Turns out, though, this partial support is kind of all that we need in rust-gpu, we don't need to represent wacky shenanigans, so I'll leave the full refactor work up to whoever needs that functionality in the future :)

Additionally, the debug section needs to be split up into its three component subsections to not intermix OpString and OpName instructions (which, OpString is relevant to OpLine because that's how the source file is specified). The spec states:

  1. These debug instructions, which must be grouped in the following order:
    a. All OpString, OpSourceExtension, OpSource, and OpSourceContinued, without forward references.
    b. All OpName and all OpMemberName.
    c. All OpModuleProcessed instructions.

I don't know why they didn't just make it three distinct "top-level" sections, because that's essentially what these subsections are, and it just causes a lot of confusion. Anyway, unfortunately, splitting up the subsections is a breaking change, so reviewers should be aware of that and consider alternatives (but I think a breaking change is the right way to go here).

@khyperia khyperia requested a review from Jasper-Bekkers May 5, 2021 09:02
Comment on lines +37 to +42
/// Debug subsection: All OpString, OpSourceExtension, OpSource, and OpSourceContinued.
pub debug_string_source: Vec<Instruction>,
/// Debug subsection: All OpName and all OpMemberName.
pub debug_names: Vec<Instruction>,
/// Debug subsection: All OpModuleProcessed instructions.
pub debug_module_processed: Vec<Instruction>,
Copy link

Choose a reason for hiding this comment

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

Tiny nit pick: looks like all the other names are pluralized, so I'd expect debug_string_sources and debug_module_processeds.

@Jasper-Bekkers Jasper-Bekkers merged commit 3673812 into master May 5, 2021
@khyperia khyperia deleted the partial-opline-support branch May 19, 2021 08:17
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.

4 participants