discard(), update rspirv, better capability computation #380
Conversation
|
and to answer questions about "why not just default to demote to helper invocation always, not even support OpKill", I get this error message on my modern desktop GPU with up-to-date drivers:
|
| } | ||
|
|
||
| /// Calls the `OpKill` instruction, which corresponds to discard() in GLSL | ||
| pub fn discard() { |
There was a problem hiding this comment.
To be consistent with the other function, should we have this one called kill so that it matches the convention of name of instruction except Op.
| pub fn discard() { | |
| pub fn kill() { |
There was a problem hiding this comment.
I want to match user's expectations of having a function/intrinsic called discard(). Not having it seems problematic, and causes unnecessary friction - blindly copying the names of SPIR-V things is something we'd like to avoid (@Jasper-Bekkers believes this much stronger than I, haha). I chose to default the discard() implementation to the GLSL one instead of the HLSL one because it's supported by default instead of requiring an extension (see error message).
There was a problem hiding this comment.
Something we could do is have a
mod glsl { fn discard(); }
mod hlsl { fn discard(); }
fn discard() { glsl::discard(); }not sure if that's better/worse
There was a problem hiding this comment.
blindly copying the names of SPIR-V things is something we'd like to avoid
I mean this is ostensibly this is spirv-std so I think matching their naming makes more sense rather than having a mix of semantics from different specs and languages.
If discoverability is a concern, we can add a doc alias so that when users search for discard they find this function. E.g.
#[doc(alias = "discard")]
fn kill() {}
| rspirv = { git = "https:/gfx-rs/rspirv.git", rev = "01ca0d2e5b667a0e4ff1bc1804511e38f9a08759" } | ||
| rspirv = { git = "https:/gfx-rs/rspirv.git", rev = "7111e6c5a8bb43563d280825fa65623032ee052d" } |
There was a problem hiding this comment.
Nice, looks like this resulted in some deduplication/version convergence in Cargo.lock.
| pub fn discard() { | ||
| #[cfg(target_arch = "spirv")] | ||
| unsafe { | ||
| asm!("OpKill", "%unused = OpLabel"); |
There was a problem hiding this comment.
Hah, I didn't think of this when we needed discard() for a shadertoy - I'm guessing this just bypasses rspirv::dr's representation of blocks but still results in correct SPIR-V output? I wonder if it has a risk of breaking some of the linker algorithms, tho that won't happen if it gets serialized and reparsed.
There was a problem hiding this comment.
haha yeah it's extremely cursed. only reason it works is because we serialize to object files and re-parse in the linker.
| } | ||
|
|
||
| /// Calls the `OpKill` instruction, which corresponds to discard() in GLSL | ||
| pub fn discard() { |
There was a problem hiding this comment.
Besides the naming discussion, there's two things I want to add here (tho neither should block the PR):
- would
fn discard() -> !make sense? i.e. is execution guaranteed to end, unlikedemote_to_helper_invocation? - long-term we probably want to have some kind of token to avoid being able to call this outside of a fragment shader, right? (if
demote_to_helper_invocationalso needs that, it could be ademote_to_helpermethod on aShaderInvocationtoken, which I find nice naming-wise, but that's just bikeshedding)
| pub fn demote_to_helper_invocation() { | ||
| #[cfg(target_arch = "spirv")] | ||
| unsafe { | ||
| asm!( | ||
| "OpExtension \"SPV_EXT_demote_to_helper_invocation\"", | ||
| "OpCapability DemoteToHelperInvocationEXT", | ||
| "OpDemoteToHelperInvocationEXT" |
There was a problem hiding this comment.
Oh, right, the other reason to have a token is because we could support it in the CPU runner if it just had a &AtomicBool (or just &Cell<bool>) indicating it's a helper invocation and the output shouldn't be written.
Far less sure about what to do with OpKill tho, short of a panic! with a special type that can be caught and acted upon. Anyway that's just more tangents.
We could bikeshed on the names of
discard()anddemote_to_helper_invocation(), but I hope with the doc comments it's pretty clear to users regardless of the exact names.Capability computation was apparently borked, as we were including e.g. OpCapability DerivativeControl in our shaders even without calling the derivative functions. We should probably rethink this area, the current system is still a "temporary" hack. I cleaned up the hack a little bit, but it's still pretty gross and weird.