Skip to content

Compute a collatz sequence in the compute example #623

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

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

DJMcNab
Copy link
Contributor

@DJMcNab DJMcNab commented May 18, 2021

These changes were made at the behest of @raphlinus.

The expected values can be found at https://oeis.org/A006877

Should we rename the example to compute_collatz?

I'm also planning on integrating the overflow detection from wgpu. (Edit: Done - unfortunately can't use checked_add and friends (yet?))

We also should insert the timing queries into this example, since wgpu now supports them.

@DJMcNab DJMcNab requested review from khyperia, fu5ha and VZout as code owners May 18, 2021 16:32
@DJMcNab
Copy link
Contributor Author

DJMcNab commented May 18, 2021

I will change the SpirvBuilder invocation to only use the vulkan version which they need - i.e. make only the compute example use vulkan 1.1. (Edit: Done)

I'm also not sure whether the dummy bind group still needs to exist, and ideally I'd remove the new unsafe code (i.e. using flat_map core::Array::IntoIter on u32::to_ne_bytes().) (Edit: Done)

Additionally, now that multiple things are async it might be worth threading that through further. (Edit: I've made a custom block_on function instead - it's a bit ugly but it gets the job done.

I should also create a CPU runner, because this shader is simple enough that the naïve implementation of that would work fine (it seems to me that the CPU runner could live in examples/shaders/compute/main.rs, but I'm not certain). Putting it in with the existing CPU runner would be fine, although the dependencies thereof are a bit heavy weight, since this runner doesn't need any capabilities apart from stdout access. (Edit: It's probably fine for this to wait - given that the sky shader CPU runner works, that's probably enough evidence that the build could build for CPU).

I found the change/run/execute cycle to be excruciatingly slow - am I missing some tricks I can use to get it faster? For reference, my current invocation is:

$ cargo run -p example-runner-wgpu -- --shader Compute

@@ -2,7 +2,7 @@ use spirv_builder::SpirvBuilder;
use std::error::Error;

fn build_shader(path_to_create: &str) -> Result<(), Box<dyn Error>> {
SpirvBuilder::new(path_to_create, "spirv-unknown-vulkan1.0").build()?;
SpirvBuilder::new(path_to_create, "spirv-unknown-vulkan1.1").build()?;

Choose a reason for hiding this comment

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

It's odd to me that this requires Vulkan 1.1. Playing with it, it seems it needs the VK_KHR_storage_buffer_storage_class extension, probably because the SPV refers to StorageBuffer. If you look at the GLSL version and the spv it compiles to, it looks pretty different - it's a BufferBlock.

I'm not super expert on this, but it seems like it's using more advanced features than it really needs to, which might hurt compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forget if we documented this anywhere, but a decision made in rust-gpu is to be pretty opinionated in that we should only support non-deprecated SPIR-V features, and be pretty "modern" in that sense (for now at least - perhaps when we have more fundamentals in place, we can look at supporting older things). BufferBlock is explicitly deprecated, replaced with Block/StorageBuffer, and so we don't plan on supporting BufferBlock right now, and so require vulkan 1.1+ for this.

Choose a reason for hiding this comment

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

@khyperia Good to know thanks! I've been spending a lot of time looking at compatibility recently because I'm trying to target phones, and very much appreciate the load of dealing with older stuff. Having a clean, well documented line is a good strategy.

@DJMcNab
Copy link
Contributor Author

DJMcNab commented May 19, 2021

I think that the only things left are:

@DJMcNab
Copy link
Contributor Author

DJMcNab commented May 19, 2021

If these performance counters are done correctly, then we should also add them to the other example, closing the saga of #259

I have also only now realised that I have inadvertently duplicated #360. I was just trying to show @raphlinus that rust-gpu can be used to make compute shaders 😅

@DJMcNab DJMcNab changed the title Compute the collatz sequence in the compute example Compute a collatz sequence in the compute example May 20, 2021
@DJMcNab DJMcNab requested a review from eddyb as a code owner May 20, 2021 09:38
@DJMcNab DJMcNab force-pushed the collatz branch 2 times, most recently from 97ee533 to 1c7df2a Compare May 27, 2021 15:17
@DJMcNab DJMcNab force-pushed the collatz branch 3 times, most recently from 59ccc4f to 1870129 Compare May 31, 2021 15:53
Build the compute shader for vulkan1.1 as required
@khyperia khyperia merged commit cb95256 into EmbarkStudios:main Jun 3, 2021
@DJMcNab DJMcNab deleted the collatz branch June 3, 2021 13:16
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.

5 participants