-
Notifications
You must be signed in to change notification settings - Fork 244
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
Add API to enable ext/capabilities, and remove default capabilities #630
Conversation
.iter() | ||
.any(|inst| { | ||
inst.class.opcode == Op::Extension | ||
&& inst.operands[0].unwrap_literal_string() == extension | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be O(1)
as a set of interned strings, maybe open an issue about simple algorithmic inefficiencies like this and leave // FIXME(#1234)
comments? At least that's what I would should do.
// target_features is a HashSet, not a Vec, so we need to sort to have deterministic | ||
// compilation - otherwise, the order of capabilities in binaries depends on the iteration | ||
// order of the hashset. Sort by the string, since that's easy. | ||
feature_names.sort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a FxHashSet
, right? Its order should already be deterministic. But yes this is probably a good idea just to keep the output clean, if nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be deterministic! In practice, somehow, it isn't - I hit a different order when upgrading rust-toolchain
, not sure if the actual cause of the different order was the different rustc binary, or if the different order was more common/subtle than that. (Random guess, do symbols like, use their pointer as a hash key, instead of the string contents? So if the symbols are allocated in a different order, the FxHashSet<Symbol>
order is different?)
build_shader( | ||
"../../shaders/mouse-shader", | ||
false, | ||
&[Capability::Int8, Capability::Int16, Capability::Int64], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe open an issue for this? I think it's just one of those num-traits
things like .abs()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RustGPUShader::Compute => ("compute-shader", &[]), | ||
RustGPUShader::Mouse => ( | ||
"mouse-shader", | ||
&[Capability::Int8, Capability::Int16, Capability::Int64], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, the duplication here isn't great. There's no easy way to share this information, is there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only thought was to make a common file and include!()
it from both (or #[path = "..."] mod blah;
or whatever), but I'm not sure if that makes it better or worse.
I'm going to leave #633 as just an issue instead of a comment in code |
I had to add a decent amount of debugging infrastructure to figure out wtf was going wrong when the default capabilities weren't enabled, and I figured I should clean up that infra and leave it in, so there's a decent amount of error message changes in this PR.
Also included is adding
glam
to system crates, because otherwise it barfs on all usages of e.g.f64
in glam ifFloat64
isn't enabled, and I don't want to force glam to write#[cfg(any(not(target_arch = "spirv"), target_feature = "Float64"))]
everywhere (and Int8, etc.) - it'd be a maintenance nightmare, easily broken.Also, compiletest gets all of Int8, Int16, Int64, Float64 automatically enabled for both the "library" crates (everything except for the compiletest'ed crate), as well as the final crate. I ranted about this in discord for as to why, I'm just going to copy that in instead of rewriting it: