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

Implement ByteAddressableBuffer prototype detached from bindless #735

Merged
merged 3 commits into from
Aug 31, 2021

Conversation

khyperia
Copy link
Contributor

Tests are a little lacking, but definitely better than no tests at all.

Two problems with this:

  1. It appears that we currently compile an explicit panic!() as OpUnreachable (with some strange convoluted branching/looping surrounding it), which isn't correct (e.g. spirv-opt replaces it with a straight up OpReturn)
  2. The usability is a little poor right now because the ByteAddressableBuffer type isn't allowed directly in entrypoint arguments yet - that'll be cleaned up in the future (requires tangling entry.rs up a little more, and should probably be in a separate PR cleaning up/refactoring entry.rs as well). Converting &mut [u32] <-> ByteAddressableBuffer will still be allowed, though, as that seems like a really useful feature.

I do actually want to take a step back and discuss if this is actually needed and is in line with our goals of rust-gpu, though. This type is an incredibly type-unsafe, unsound, and not-rusty way of doing things, which seems to be against the goals of what rust-gpu wants to be (safe, sound, etc.). An example of an alternative way of doing this would be to make a "loadable from bytes" trait, implemented by us in spirv-std for all the basic types, and a proc macro(?) to automatically mark/implement it for structs (similar to serde). There's probably many other alternatives. So, I just want to step back and make sure we're on the right path here, and not just implementing things because it's what other shader languages do.

@khyperia khyperia requested a review from eddyb as a code owner August 23, 2021 11:44
@hrydgard
Copy link
Contributor

I haven't thought about this much, but I could imagine that we treat these as unsafe, and the intention is that you use this functionality when it's really needed to implement some data structure on the GPU, and you're supposed to wrap it yourself to make it safe.

This kind of thing is probably perfect for storing complicated things like bounding volume hierarchies where nodes and leaves are different structs, etc.

@eddyb
Copy link
Contributor

eddyb commented Aug 23, 2021

My best guess for the problem of spirv-opt optimizing our intrinsics::abort() infinite loop into a return would be this PR:

It claims it's UB to be in that situation but I haven't combed the standard for the exact reason why - OTOH we've seen this kind of UB driver-side (presumably because of the use of LLVM older than LLVM 12) on at least Nvidia/Windows, months ago.

It's pretty old, so the only reasons we haven't noticed are:

  1. we weren't running spirv-opt back when panic! was first added
  2. even today we don't enable optimizations for the disassembly tests
    • we could use the compiletest "revisions" feature to have both unoptimized and optimized output for every disassembly test, so that we know what it optimizes to

@Jasper-Bekkers
Copy link
Contributor

Thanks for adding this back, though I have to say it feels weird to see this fully attributed to you now since most of this got introduced in #450 initially and subsequently removed by you in #710. It looks like some code got removed (support for u64 for example) and shuffled around in the meantime though.

This type is an incredibly type-unsafe, unsound, and not-rusty way of doing things, which seems to be against the goals of what rust-gpu wants to be (safe, sound, etc.).

I agree and disagree with some of these and have some points to add on this as well. I think it's very rust-y to have unsafe (and even unsound) pieces of code, especially when related to this code, which is basically emulating the behavior of pointers to a certain extent. A feature that's pretty core to the foundations of Rust. Especially because this type is supposed to serve as an unsafe, low level abstraction on top of which safe(r) abstractions can be built.

I think the name ByteAddressBuffer as used in HLSL is already a bit of a misnomer (since the name kinda, sorta can be interpreted to mean "byte aligned buffer" which it isn't). I'd like to suggest something along the lines of UnsafeBuffer or UnsafeAddressBuffer (with both load & store marked as unsafe) for our use-cases here. What do you think?

An example of an alternative way of doing this would be to make a "loadable from bytes" trait, implemented by us in spirv-std for all the basic types, and a proc macro(?) to automatically mark/implement it for structs (similar to serde).

This sounds conceptually quite close to for example TriviallyTransmutable which feels like a great abstraction to have on the higher level API.

Similarly, in #450 I added a few types that were safer to operate - for example a SimpleBuffer<T> that would just load & store to offset 0 at all times, and a ArrayBuffer<T> that would index off of multiples of std::mem::size_of::<T>(), I'm not proposing adding these back in this PR, but at least it shows the intended use case. Also notice that while ArrayBuffer<T> and a regular buffer seem like they may have very similar interfaces & use cases, there are some key differences (ArrayBuffer<T> didn't need to be declared as part of the kernel arguments, being the main one, since it would always just alias the globally bound u32 buffer meaning we could just have one binding for buffers in total).

Co-authored-by: Markus Siglreithmaier <m.siglreith@gmail.com>
Comment on lines +28 to +33
/// This can only happen in one specific case - which is as a result of
/// `codegen_buffer_store_intrinsic`, that function is supposed to return
/// OpTypeVoid, however because it gets inline by the compiler it can't.
/// Instead we return this, and trigger an error if we ever end up using the
/// result of this function call (which we can't).
IllegalTypeUsed(Word),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why this is needed, wouldn't an undef work? I thought that was what was used in other similar cases. It's not that important though, feel free to merge without touching this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah, probably, a zombie'd undef would work (I'm guessing you can't have an undef void) - this was directly carried over from the previous implementation, though, didn't really think about it that much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, there's some discussion around the original implementation here #450 (although quite a bit of discussion isn't actually in that PR, due to instead having calls due to frustration around PR stuff)

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe what happens with the other cases is they make an undef () (i.e. the ZST Rust type, an OpTypeStruct without fields), and OpTypeVoid appears here only because it was intentionally specified.

This gives me an idea I could try (removing SpirvType::Void and using Option where necessary instead) - tho because rspirv doesn't bother interning types, it would be unnecessarily inefficient.

Comment on lines +69 to +72
/// Intrinsic for loading a <T> from a &[u32]
pub buffer_load_intrinsic_fn_id: RefCell<FxHashSet<Word>>,
/// Intrinsic for storing a <T> into a &[u32]
pub buffer_store_intrinsic_fn_id: RefCell<FxHashSet<Word>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: this is a set (and not just a Cell<Option<_>>) because it's recording SPIR-V IDs, not DefIds, so all of the monomorphizations are different (even if they share one DefId).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, thought about that when porting it over (was confused myself), really should have added a comment about that instead of copying stuff without thinking, haha

@khyperia khyperia merged commit 66d6c55 into main Aug 31, 2021
@khyperia khyperia deleted the byte-addressable-buffer branch August 31, 2021 08:26
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