DRAFT: CPU Kernel #12
Conversation
| - uses: actions/checkout@v4 | ||
| - run: rustup toolchain install $RUST_VERSION --profile minimal | ||
| - run: | | ||
| rustup toolchain add nightly-2025-10-20 --profile minimal |
There was a problem hiding this comment.
Now that we have a rust-toolchain.toml it's enough just to have rustup toolchain install --profile minimal here and it automatically gets the version from the file. And we don't need the stable toolchain either right?
| #[expect(clippy::as_conversions, reason = "u32 -> usize is valid")] | ||
| let surfaces = cpu::multithreaded_kernel( | ||
| &elevations, | ||
| self.dem.max_los_as_points as usize, |
There was a problem hiding this comment.
You can actually just do: usize::try_from(self.dem.max_los_as_points)? here and no need for the lint exception.
|
|
||
| let (sin, cos) = (f64::sin(angle.to_radians()), f64::cos(angle.to_radians())); | ||
|
|
||
| #[expect(clippy::integer_division, reason = "we don't need precision here")] |
There was a problem hiding this comment.
Let me think about this a bit harder, euclid division may actually be the correct behavior for this one
crates/total-viewsheds/src/cpu.rs
Outdated
| clippy::integer_division, | ||
| reason = "i32 is constructed from (i32, i32) converting back should succeed" | ||
| )] | ||
| let x = (val / width as i32) - max_los as i32; |
There was a problem hiding this comment.
I wonder if some of these as conversions can just be i32::from(...) or i32::try_from(...)??
There was a problem hiding this comment.
I'll try it out, but there are some weird issues regarding isize and i32s stopping a from. try_from is a perf penalty due to a panic if the value is out of bounds.
Let me double check on why I can't just do from
There was a problem hiding this comment.
usize/isize may be 64 bits, so there would be truncation so no on from. Still agree that try_from is a perf hit. If we could make it debug only, I'd welcome it.
| @@ -0,0 +1,3 @@ | |||
| [toolchain] | |||
There was a problem hiding this comment.
Would be good to have a comment about what features we need from this channel and version. And maybe a Github issue to watch so we know when it reaches stable?
There was a problem hiding this comment.
Portable simd is what will be keeping us on nightly, and I don't know if they ever will stabilize it. I wouldn't count on it for another few years likely
|
The only issue I see with this at the moment is that it's currently running your CPU implementation and then my old CPU implementation right afterwards! Ideally I think you should just replace my implementation. But really that should mean that your implementation is producing viewsheds and hooked into the tests. So for now, what about if we just add a new backend like |
I don't think it runs both one after another. Notice the early return inside of the We can definitely create a new backend though! |
|
Oh sorry, I missed the early return! So something like this then: match self.config.backend {
crate::config::Backend::CPUNext => cpunext(),
crate::config::Backend::CPU | crate::config::Backend::Vulkan => oldcode(),
crate::config::Backend::Cuda => todo!(),
}That should fix the E2E test too. Then I would say, that if you wanted, you could merge it. But no rush. |
This reverts commit 2e16cb3.
Co-authored-by: Ryan Berger <[email protected]>
No description provided.