Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
eddyb
left a comment
There was a problem hiding this comment.
Nitpick: [# everywhere should be replaced with #[.
(the # is the attribute syntax, [ is merely a "container" - this is similar to how some languages have @foo and @bar(123) attributes but that @ is # in Rust and brackets exist to disambiguate something like #[allow(overflowing_literals)] (x + 200i8) which without the brackets would me ambiguous as to whether the parens are attached to allow - the languages that don't need that tend to be very limited in where you can apply attributes - anyway Rust can look like languages with [foo] attribute syntax but AFAIK it's closer to the @foo languages despite the aesthetics...)
This comment was marked as resolved.
This comment was marked as resolved.
fu5ha
left a comment
There was a problem hiding this comment.
I didn't look this over exhaustively but the gist of the change is a 👍 from me
|
Waiting for #929 first |
This PR adds the commented out use spirv_std::spirv; to invalid-target.rs in preparation for #926
- We now always apply a proc_macro_attribute `spirv` that emits target-conditional `rust_gpu::spirv` attributes. - Moved `no_std`, `feature(register_tool)` and `register_tool(rust_gpu)` to `SpirvBuilder`'s standard compile options
- Included `use spirv_std::spirv` in all tests - Added `no_std`, `feature(register_tool)` and `register_tool(spirv)` to test dep compiler options
* Rearranged attrbute code parsing, added error for `#[rust_gpu::*]` cases where `*` is not `spirv` * Removed `no_std` from standard rust-gpu crate attributes, readded it to `spirv-std`/`spirv-std-types` * Removed (now useless) `register_tool(rust_gpu)` from examples * Blessed one changed test
There was a problem hiding this comment.
These are the only files I didn't check off as "viewed":
$$('.js-reviewed-checkbox').filter(x=>!x.checked).map(x=>x.parentNode.parentNode.path.value).join('\n')tests/ui/arch/all.rs
tests/ui/arch/any.rs
tests/ui/arch/convert_u_to_acceleration_structure_khr.rs
tests/ui/arch/demote_to_helper_invocation.rs
tests/ui/arch/emit_stream_vertex.rs
tests/ui/arch/emit_vertex.rs
tests/ui/arch/end_primitive.rs
tests/ui/arch/end_stream_primitive.rs
tests/ui/arch/execute_callable.rs
tests/ui/arch/ignore_intersection_khr.rs
tests/ui/arch/kill.rs
tests/ui/arch/report_intersection_khr.rs
tests/ui/arch/terminate_ray_khr.rs
tests/ui/arch/trace_ray_khr.rs
tests/ui/image/issue_527.rs
But looks like GH decided to merge it anyway? :/
EDIT: those ended up being fixed by:
|
|
||
| #[cfg_attr(not(target_arch = "spirv"), macro_use)] | ||
| #[macro_use] | ||
| pub extern crate spirv_std_macros as macros; |
There was a problem hiding this comment.
You marked my previous comment asking about the pub here as resolved, but I don't remember what the explanation was, sorry - it's just that spirv_std::macros::... is used elsewhere, right?
There was a problem hiding this comment.
Oh I did this because the things in spirv-std now need spirv_std::spirv to be visible for all platforms. This way I didn't need to write use spirv_std::spirv; in every single file.
|
|
||
| please remove the conditional attribute. | ||
|
|
||
| For this macro attribute to work correctly, it is important that `spirv` is visible in the global score and you use it like you used it before: `#[spirv(..)]`. An attempt to scope the attribute (such as `#[spirv_std::spirv(..)]`) will confuse the macro and likely fail. |
There was a problem hiding this comment.
We don't need to expand on this much more but to be clear this only applies to weird "leaf" things like function args, fields, etc. (which are replaced while transforming a larger thing that triggered the proc macro as usual).
Whole-definition attributes (e.g. the whole entry-point or a whole struct) will just work through normal name resolution.
Funny thing about that btw, if you only need it for a leaf, you need to do:
#[spirv()]
fn foo(
#[spirv(ray_tracing_weirdness)] x: T,
) {}(with the whole-fn one only serving to handle the inner ones lol - again, I don't think we have to mention this, since we don't have that many attributes to begin with, and so far such a usecase doesn't exist AFAIK, but I wanted to expand on it "for the record" so to speak)
There was a problem hiding this comment.
If we do end up having a case where this applies (I don't think we do currently?), I'd suggest supporting #[spirv] without the parens.
There was a problem hiding this comment.
I'd suggest supporting
#[spirv]without the parens.
I don't think we have a say in the matter (i.e. both should work), and you likely can't tell them apart (since that's the proc macro, not the tool module one).
We need this because the
remove_attrfeature was removed from the Rust nightly early September