versionize rust_gpu tool to ensure codegen and spirv-std versions match
#401
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Firestar99
force-pushed
the
tool_rename
branch
2 times, most recently
from
662f4be to
5b7acff
Compare
Firestar99
changed the title
rust_gpu to rustc_codegen_spirvrust_gpu tool to ensure codegen and spirv-std versions match
| // NOTE(shesp) Directly using `#[rust_gpu::spirv(...)]` because macro attributes are invalid in most contexts | ||
| // NOTE(shesp) Directly using `#[rust_gpu::spirv_v0_9(...)]` because macro attributes are invalid in most contexts | ||
|
|
||
| #[rust_gpu::spirv( |
There was a problem hiding this comment.
This test is a problem. We can't replace every use with our spirv() proc macro, since some of these are in locations proc macros are illegal to place. Ideas?
There was a problem hiding this comment.
Would it help to put a #[spirv()] (invoking the proc macro, but in a dummy way) on the outermost definition, in those cases (or even a whole module, though I think that might still be disallowed).
There's some pretty aggressive rewriting of any #[spirv(...)] anywhere inside the token stream, but I'm not sure what the limitations of that are.
If the empty list is an issue, we could make up something for this test, or just allow it as a noop.
There was a problem hiding this comment.
Currently, the proc macro searches for the "regex" (#[spirv(.*)]) non-recursively. This includes the parenthesis on the outside (to filter for function params) and does not care for what comes after the spirv ident, it just replaces that with #[cfg_attr(target_arch = "spirv", rust_gpu::spirv($1))].
We could make it recurse properly, but I'm also slightly concerned about performance, if we were to attach that macro to a big function like entry points.
Firestar99
force-pushed
the
tool_rename
branch
from
5b7acff to
d37bf4e
Compare
|
@eddyb this is ready for review! Since the first one just moved a bunch of code, best reviewed without it or commit by commit. Open Questions
|
crates/spirv_attr_version.rs
Outdated
| //! This is placed outside any crate, and included by both `spirv_std` and `rustc_codegen_spirv`. | ||
| //! I could have made a new crate, shared between the two, but decided against having even more small crates for sharing | ||
| //! types. Instead, you get this single small file to specify the versioned spirv attribute. |
There was a problem hiding this comment.
Can you try cargo publish -p spirv-std-macros --dry-run?
I don't see how this can work with crates.io publishing (this is the same reason crates/rustc_codegen_spirv/build.rs cannot just include_str!("../../rust-toolchain.toml")).
(Would be very happy to be wrong, and I do know that Cargo git dependencies result in the whole repo being checked out, it's just registry publishing we need to be careful about)
There was a problem hiding this comment.
fails... will figure something out
There was a problem hiding this comment.
Moved it to spirv-std-types / spirv-std/shared, gave that crate an std feature and have rustc_codegen_spirv depend on it too.
Publish still fails as for some reason it wants to use the published spirv-std-types v0.9.0 instead of the local one, which doesn't have the std feature.
Just the test remains...
Firestar99
force-pushed
the
tool_rename
branch
2 times, most recently
from
febdddf to
bdc2048
Compare
|
@eddyb this is ready for round 2 of reviews! (most of the diff's LOC is blessing the |
Firestar99
force-pushed
the
tool_rename
branch
from
bdc2048 to
3cfe38a
Compare
As discussed with @eddyb, we would like to versionize our
spirv()macro so you can't accidentally combine two incompatible versions.Also enables the addition of "stable spirv macros" that we can offer backward compat for. This will be used in #380 to allow
glamto define it'sVecNas rust-gpu vectors, for which it must refer to our "rustc tool"rust_gpudirectly instead of going via ourspirv()proc macro, which expands to our tool name.