Skip to content

enum SpirvTargetEnv containing all available targets #311

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

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

Conversation

Firestar99
Copy link
Member

@Firestar99 Firestar99 commented Jul 1, 2025

cargo-gpu fixup: Rust-GPU/cargo-gpu#92

List all available targets in enum SpirvTargetEnv and centralize parsing, to_string and target spec references there. Changes SpirvBuilder.target from String to the new enum and makes SpirvBuilder::new accept both String and the new enum to reduce breakage as much as possible.

@Firestar99 Firestar99 marked this pull request as draft July 1, 2025 11:32
@Firestar99 Firestar99 force-pushed the target_env_enum branch 4 times, most recently from 326f31a to 669460a Compare July 1, 2025 15:51
@Firestar99 Firestar99 force-pushed the target_env_enum branch 2 times, most recently from 109b8c9 to 4be9bba Compare July 1, 2025 17:23
@@ -490,10 +490,10 @@ impl Default for SpirvBuilder {
}

impl SpirvBuilder {
pub fn new(path_to_crate: impl AsRef<Path>, target: impl Into<String>) -> Self {
pub fn new(path_to_crate: impl AsRef<Path>, target: SpirvTargetEnv) -> Self {
Copy link
Member Author

@Firestar99 Firestar99 Jul 1, 2025

Choose a reason for hiding this comment

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

This PR introduces a significant breaking change right here that will affect everyone. I would like to change SpirvBuilder.target to the new enum, so new users don't have to deal with strings. But this moves parsing of the target string from build() to here, where we can't just return an error easily. The options:

  • breaking change: return a Result for failed parsing, may be annoying to handle for the user
  • My choice: breaking change: require everyone to use the new enum directly (SpirvTargetEnv::Vulkan_1_2) or explicitly call the parse function that may fail (SpirvTargetEnv::parse_triple("spirv-unknown-vulkan1.2")?)
  • a surprise panic if parsing fails

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not impl Into<SpirvTargetEnv> and support &'static str, breaking everyone who didn't specify a string?

I forget if you can mark an impl or its methods as deprecated (or rather, if that ever results in a warning).

But honestly, enumerating the target-env explicitly is a bit silly. It would be much cooler if you can come up with a foolproof way for spirv-builder itself to create and cache a target JSON (e.g. in the target dir, which currently we only detect based off of $OUT_DIR, but maybe we could cheaply get "what would be the target dir used for the shader crate" out of Cargo?).

And then we presumably wouldn't even need this part of the system to care about the values at all and it can remain a string (unless that interferes with other plans ofc).

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have an even better idea how not to break anything: Just have SpirvBuilder.target be Result<SpirvTargetEnv, *ParseError> so we can forward the error to the build() call. Then make new(..., target: impl IntoSpirvTargetEnv) so it can accept a &str, String or SpirvTargetEnv. We would only break anyone doing builder.target = Some("spirv-unknown-vulkan1.2") which is a small minority since that only works since about 2 months or so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

impl IntoSpirvTargetEnv ... so it can accept a &str, String or ...

I suppose one way to be 100% back-compat would be to implement IntoSpirvTargetEnv for all types that implement Into<String>, but that might feel wasteful, idk. Then again, some of this depends on whether we care about parsing SpirvTargetEnv at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that IDEs can auto-complete enum names, since they're symbols, not magic strings

@Firestar99 Firestar99 marked this pull request as ready for review July 1, 2025 17:50
Comment on lines +7 to +62
match self {
SpirvTargetEnv::OpenGL_4_0 => {
include_str!("../target-specs/spirv-unknown-opengl4.0.json")
}
SpirvTargetEnv::OpenGL_4_1 => {
include_str!("../target-specs/spirv-unknown-opengl4.1.json")
}
SpirvTargetEnv::OpenGL_4_2 => {
include_str!("../target-specs/spirv-unknown-opengl4.2.json")
}
SpirvTargetEnv::OpenGL_4_3 => {
include_str!("../target-specs/spirv-unknown-opengl4.3.json")
}
SpirvTargetEnv::OpenGL_4_5 => {
include_str!("../target-specs/spirv-unknown-opengl4.5.json")
}
SpirvTargetEnv::Spv_1_0 => {
include_str!("../target-specs/spirv-unknown-spv1.0.json")
}
SpirvTargetEnv::Spv_1_1 => {
include_str!("../target-specs/spirv-unknown-spv1.1.json")
}
SpirvTargetEnv::Spv_1_2 => {
include_str!("../target-specs/spirv-unknown-spv1.2.json")
}
SpirvTargetEnv::Spv_1_3 => {
include_str!("../target-specs/spirv-unknown-spv1.3.json")
}
SpirvTargetEnv::Spv_1_4 => {
include_str!("../target-specs/spirv-unknown-spv1.4.json")
}
SpirvTargetEnv::Spv_1_5 => {
include_str!("../target-specs/spirv-unknown-spv1.5.json")
}
SpirvTargetEnv::Spv_1_6 => {
include_str!("../target-specs/spirv-unknown-spv1.6.json")
}
SpirvTargetEnv::Vulkan_1_0 => {
include_str!("../target-specs/spirv-unknown-vulkan1.0.json")
}
SpirvTargetEnv::Vulkan_1_1 => {
include_str!("../target-specs/spirv-unknown-vulkan1.1.json")
}
SpirvTargetEnv::Vulkan_1_1_Spv_1_4 => {
include_str!("../target-specs/spirv-unknown-vulkan1.1spv1.4.json")
}
SpirvTargetEnv::Vulkan_1_2 => {
include_str!("../target-specs/spirv-unknown-vulkan1.2.json")
}
SpirvTargetEnv::Vulkan_1_3 => {
include_str!("../target-specs/spirv-unknown-vulkan1.3.json")
}
SpirvTargetEnv::Vulkan_1_4 => {
include_str!("../target-specs/spirv-unknown-vulkan1.4.json")
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick question since we keep changing these things, since I went with JSON files to minimal deviation from previous behavior, and because they wouldn't cause spurious rebuilds (relative to the spirv-builder version being used, since I would expect shaders to be rebuilt most of the times spirv-builder itself is rebuilt):

Could cargo-gpu generate the JSON file on-demand (and maybe cache it)? Especially since there's include_str! going on here, I feel like that's already what's happening?

Because the only difference between these files is, for spirv-unknown-X:

  • env is set to X
  • llvm-target is set to spirv-unknown-X
$ echo crates/rustc_codegen_spirv-target-specs/target-specs/*.json | xargs -n1 diff -U0 crates/rustc_codegen_spirv-targe
t-specs/target-specs/spirv-unknown-spv1.0.json | rg '^[\-+] ' -r '' | sed 's/: .*/: .../' | sort -u
 "env": ...
 "llvm-target": ...

(run this without sed 's/: .*/: .../' to see the actual values)

They could even be generated by string interpolation, if not serde_json, since the target env name doesn't even need escaping!

Copy link
Member Author

Choose a reason for hiding this comment

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

cargo-gpu only needs them for the "legacy path" aka rustc_codegen_spirv versions which did not depend on *target-specs. And that "legacy" dependency is fixed to version 0.9 on crates, so technically these changes don't actually do anything. It just feels wrong not to adjust them as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm not 100% sure what's going on, are you saying that most of the time, the .json files in the source tree get used?

I would actually be happier if cargo-gpu/spirv-builder created these from one template - I don't mind rustc_codegen_spirv-target-specs existing (and we can release older versions of that crate, if we want, I guess?) - but its main job could be "take this target_env string and return a JSON blob", without having the result of that templating be baked in the way it is today.

Copy link
Member Author

@Firestar99 Firestar99 Jul 1, 2025

Choose a reason for hiding this comment

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

Wow, I actually wrote some nice docs for what's going on: https://github.com/Rust-GPU/cargo-gpu/blob/main/crates/cargo-gpu/src/target_specs.rs

And I get that they actually don't change much, just something I didn't know / want to worry about back when I wrote this.

@Firestar99 Firestar99 marked this pull request as draft July 1, 2025 20:30
@Firestar99 Firestar99 self-assigned this Jul 1, 2025
@Firestar99 Firestar99 marked this pull request as ready for review July 1, 2025 20:48
@LegNeato
Copy link
Collaborator

LegNeato commented Jul 5, 2025

@eddyb are we good to merge here?

@LegNeato
Copy link
Collaborator

FWIW I like types much better than strings and I feel centralizing is super valuable.

@eddyb
Copy link
Collaborator

eddyb commented Jul 12, 2025

FWIW I like types much better than strings and I feel centralizing is super valuable.

My issue with this is that nothing other than rustc_codegen_spirv really counts as "central enough" for this, and enumerating the possibilities feels unnecessarily rigid compared to templating the target spec JSON.

I still need to look at the docs linked in #311 (comment) - I just missed that comment until today, sorry!

@Firestar99 Firestar99 force-pushed the target_env_enum branch 3 times, most recently from 0891715 to cc372c5 Compare July 26, 2025 09:35
@Firestar99 Firestar99 enabled auto-merge July 26, 2025 09:38
Comment on lines +68 to +87
#[test]
fn test_serde_target_roundtrip() {
#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
struct FakeSpirvBuilder {
#[serde(
serialize_with = "serialize_target",
deserialize_with = "deserialize_target"
)]
target: Option<Result<SpirvTargetEnv, SpirvTargetParseError>>,
}

for env in SpirvTargetEnv::iter() {
let builder = FakeSpirvBuilder {
target: Some(Ok(env)),
};
let json = serde_json::to_string(&builder).unwrap();
let deserialize: FakeSpirvBuilder = serde_json::from_str(&json).unwrap();
assert_eq!(builder, deserialize);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh since you're adding tests like this, it might be worth adding one for the actual target spec JSONs (they should all deserialize just fine into serde_json::Values or w/e, and then only differ by the target env, which appears in exactly two places).

Tbh even target_spec.replace(target_env, "___ENV___") might be enough to check (but it wouldn't guarantee where the target env appears).

@eddyb
Copy link
Collaborator

eddyb commented Jul 26, 2025

@LegNeato wrote in #311 (comment):

FWIW I like types much better than strings and I feel centralizing is super valuable.

@Firestar99 wrote in #311 (comment):

I like that IDEs can auto-complete enum names, since they're symbols, not magic strings

And I kind of get that sentiment (having expressed it myself in many occasions), but there are reasons I really dislike the full set of target env options (on top of this crate not being a source of truth).

The values we accept, and their implications, are:

SPIR-V
version
Memory
model
SPIR-V spv1.0
spv1.1
spv1.2
spv1.3
spv1.4
spv1.5
spv1.6
1.0
1.1
1.2
1.3
1.4
1.5
1.6
Simple
OpenGL opengl4.0
opengl4.1
opengl4.2
opengl4.3
opengl4.5
1.0 GLSL450
Vulkan vulkan1.0
vulkan1.1
vulkan1.1spv1.4
vulkan1.2
vulkan1.3
vulkan1.4
1.0
1.3
1.4
1.5
1.6
1.6
Vulkan

In theory, there are also OpenCL 1.2 and 2.{0,1,2} (and "embedded" variants thereof), but we don't offer JSONs for those (and generally don't support their semantics, which differ enough from Vulkan to complicate everything).


If you stare at that table, and consider actual usecases:

  • spv1.x is probably a bad idea outside of testing (it removes useful validation)
  • opengl4.x might be unused (but they're all SPIR-V 1.0 and just a version number)
  • vulkan1.x is the main one users should rely on and it's a Vulkan version
    • that is, users should be plugging in the minimum version number they plan to support
      (or for tiered support, you might build both vulkan1.1 and vulkan1.4 shaders)
    • the only outlier is vulkan1.1spv1.4 (presumably due to vulkan1.2 spec/support delays?)

Honestly, we could even replace spirv-unknown-vulkan1.1spv1.4 with spirv-unknown-vulkan1.1 w/ +spv1.4 as a target feature, then most target envs most ppl would ever have to engage with, would be Vulkan versions.


To summarize, I have several (somewhat related) concerns:

  • we have N copies of the same JSON file where the only difference is the target env string
  • don't like seeing an enum w/ the set of target envs (a weird ad-hoc thing Khronos made up)
  • anything user-facing could focus on "pick a Vulkan version" instead of strings or enums

Maybe I could accept a compromise, for now, which looks more like this:

pub enum SpirvTargetEnv {
    // (I considered some deduplication but it's not worth the noise IMO)
    OpenGL4 { version_minor: u8 },
    Vulkan1 { version_minor: u8 },

    Custom(Cow<'static, str>),
}

impl SpirvTargetEnv {
    pub const VULKAN_1_0: Self = Self::Vulkan1 { version_minor: 0 };
    pub const VULKAN_1_1: Self = Self::Vulkan1 { version_minor: 1 };
    pub const VULKAN_1_1_SPIRV_1_4: Self = Self::Custom(Cow::Borrowed("vulkan1.1spv1.4"));
    pub const VULKAN_1_2: Self = Self::Vulkan1 { version_minor: 2 };
    pub const VULKAN_1_3: Self = Self::Vulkan1 { version_minor: 3 };
    pub const VULKAN_1_4: Self = Self::Vulkan1 { version_minor: 4 };
}

(this also made me realize we spell out vulkan but spv is shortened... if we weren't using the SPIRV-Tools names, we could've really gone with the much snapper e.g. vk1.2)


Last but not least, an update on #311 (comment):
I finally managed to look at the cargo-gpu side of things, and it sounds like only the historical rustc_codegen_spirv-target-specs 0.9.0 already published on crates.io, needs the include_str feature, and it can just be removed on main?

If we're stuck with "point rustc at the JSON file within rustc_codegen_spirv-target-specs", for both spirv-builder, and cargo-gpu itself, I could see why we might want to postpone generating all specs from one template.

But I still don't like hardcoding this arbitrary list of versions, and it's a shame the list of target specs forces our hand.

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.

3 participants