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

Add API to enable ext/capabilities, and remove default capabilities #630

Merged
merged 3 commits into from
May 26, 2021

Conversation

khyperia
Copy link
Contributor

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 if Float64 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:

uugggh, so, I'm nuking the "default Int8 capability", and running into really really annoying things in compiletest. The "standard library" crates in compiletest (everything except the end crate) is only compiled once, for every test. Say, it's compiled without the Int8 capability, and a test does enable the Int8 capability. This means that in the "standard library" crates, Int8 is zombie'd, however, in the test crate, it is not zombied. This means that if, say, the test crate calls a stdlib function taking a u8, the export in the stdlib has a zombie symbol, but the import does not have a zombie symbol, and so there's a linker type mismatch because two identical types where one is zombie and the other are not are considered distinct types. But, even if we merged, say, only Int8 types where one is zombie and the other isn't, the zombie status of the stdlib would "leak" into the end crate, and u8 would get zombie'd even if the test enables the Int8 capability.

So uh, running this by [eddyb] for sanity: I've thought about it a bit and I think the best solution is to enable all of Int8,Int16,Int64,Float64 for the compiletest "stdlib" and every test, to make sure there's no mismatches. It does mean that testing that particular part of the capability system would be impossible (since it's always enabled, we can't test an expected-failure), but, it should be fine, idk. This situation won't ever happen in the real world, since all crates in the graph are always compiled with the same capabilities.

@khyperia khyperia requested a review from eddyb as a code owner May 24, 2021 08:23
@khyperia khyperia requested review from fu5ha and VZout as code owners May 25, 2021 11:10
@khyperia khyperia linked an issue May 25, 2021 that may be closed by this pull request
Comment on lines +429 to +433
.iter()
.any(|inst| {
inst.class.opcode == Op::Extension
&& inst.operands[0].unwrap_literal_string() == extension
})
Copy link
Contributor

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.

Comment on lines +107 to +110
// 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();
Copy link
Contributor

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.

Copy link
Contributor Author

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],
Copy link
Contributor

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()?

Copy link
Contributor Author

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],
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@khyperia
Copy link
Contributor Author

I'm going to leave #633 as just an issue instead of a comment in code so I don't have to run 45-minute CI again

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.

Capability computation UX
2 participants