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

Refactorize spv{Opcode,Operand,ExtInst}TableGet to remove the target env #5332

Open
Keenuts opened this issue Jul 18, 2023 · 2 comments
Open

Comments

@Keenuts
Copy link
Contributor

Keenuts commented Jul 18, 2023

The assemble grammar can be used to retrieve valuable spec information, like the extensions required for a given capability.
Most of the code to support it seems present, but it seems like we have only one table for operands, and we ignore the spirv version in use.

This means, no matter the SPIRV version, the Capability MultiView will require the SPV_KHR_multiview extension. I don't think this is a big issue, but would be a nice improvement the the capability trimming pass incoming.

Headers are not split by version anymore for the reasons explained below. This makes the table returned valid for all target env, as long as we check the minVersion/lastVersion values.
Those function should probably not take the target env anymore (and the caller maybe) as this can be confusing.

@dneto0
Copy link
Collaborator

dneto0 commented Jul 18, 2023

Background: When we originally made this, we had separate tables for each SPIR-V version. That was going to blow up space badly, for one thing. At some point, the SPIR-V spec itself moved to a single document to cover all versions, which is now the "unified" spec, and described by the headers in spirv-headers/include/unified1/...
The spec now has annotations of "missing before" versions.
The grammar now has "version" to say what SPIR-V version includes the functionality in core.

This means, no matter the SPIRV version, the Capability MultiView will require the SPV_KHR_multiview extension. I don't think this is a big issue, but would be a nice improvement the the capability trimming pass incoming.

The operand-kinds data has two fields to allow validating the version.
See spv_operand_desc_t in source/table.h

  // Minimal core SPIR-V version required for this feature, if without
  // extensions. ~0u means reserved for future use. ~0u and non-empty extension
  // lists means only available in extensions.
  const uint32_t minVersion;
  const uint32_t lastVersion;

The "version" field in the grammar gets mapped to minVersion. And ViewIndex is marked with "version" 1.3.

So all the data is encoded already in the single unified table. Validation has to pay attention to the minVersion and maxVersion.

So I think this issue is mistakenly filed.

@Keenuts
Copy link
Contributor Author

Keenuts commented Jul 18, 2023

Ah, thanks for the context.

Right, this makes sense, and I do see the version fields to inform about that.
So it's just the code which still requires a target_env, but ignores it because it's not useful anymore...

Changing this issue to be a cleanup then.

@Keenuts Keenuts changed the title spvOperandTableGet() ignores the target environment. Refactorize spv{Opcode,Operand,ExtInst}TableGet to remove the target env Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants