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

Fragment shader just copying values doesn't compile #185

Open
Firestar99 opened this issue Dec 19, 2024 · 11 comments
Open

Fragment shader just copying values doesn't compile #185

Firestar99 opened this issue Dec 19, 2024 · 11 comments
Labels
bug Something isn't working

Comments

@Firestar99
Copy link
Member

Firestar99 commented Dec 19, 2024

Expected Behaviour

The following code snippet defines 3 fragment shaders, but the middle one does not get compiled / emit an entry point / generates a spv (with multibuilder true). Looks like it's somehow getting inlined / optimized away before monomorphization, so it's not picked up as an entry point?

#[spirv(fragment)]
pub fn fragment_works(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color +1.;
}

#[spirv(fragment)]
pub fn fragment_doesnt(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color;
}

/// causes a warning by rust-gpu about having to be inlined anyway
#[inline(never)]
#[spirv(fragment)]
pub fn fragment_works_again(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color;
}

Found while investigating an issue with hadronized on discord.

System Info

  • cargo-gpu main
  • I have no idea what rust-gpu version this is, cargo-gpu doesn't tell, apparently it's pointing to the git repo but it can't be main as the toolchains would mismatch.
@Firestar99 Firestar99 added the bug Something isn't working label Dec 19, 2024
@eddyb
Copy link
Collaborator

eddyb commented Dec 19, 2024

Adding those entry-points to examples/shaders/sky-shader/src/lib.rs lets me try:

$ git show -s --oneline
f069c58c0c0 (HEAD, upstream/main, main) examples/runners/wgpu: avoid holding onto to multiple surfaces at the same time.
$ cargo run -p multibuilder --release --no-default-features --features use-installed-tools 2>&1 | rg -U 'CompileResult \{[\s\S]+\}'
CompileResult {
    entry_points: [
        "fragment_doesnt",
        "fragment_works",
        "fragment_works_again",
        "main_fs",
        "main_vs",
    ],
    module: MultiModule(
        {
            "fragment_doesnt": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_doesnt.spv",
            "fragment_works": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_works.spv",
            "fragment_works_again": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_works_again.spv",
            "main_fs": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/main_fs.spv",
            "main_vs": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/main_vs.spv",
        },
    ),
}

The entry-points seem to all be there:

$ echo target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/*.spv | xargs -t -n1 spirv-dis | rg OpEntryPoint
spirv-dis target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_doesnt.spv
               OpEntryPoint Fragment %1 "fragment_doesnt" %2 %3
spirv-dis target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_works.spv
               OpEntryPoint Fragment %1 "fragment_works" %2 %3
spirv-dis target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_works_again.spv
               OpEntryPoint Fragment %1 "fragment_works_again" %2 %3
spirv-dis target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/main_fs.spv
               OpEntryPoint Fragment %2 "main_fs" %gl_FragCoord %4
spirv-dis target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/main_vs.spv
               OpEntryPoint Vertex %1 "main_vs" %gl_VertexIndex %gl_Position

And we can look at the generated .spv files to confirm:

$ cargo install naga-cli
    Updating crates.io index
     Ignored package `naga-cli v23.0.0` is already installed, use --force to override
$ naga target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_doesnt.{spv,wgsl}
$ cat target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/fragment_doesnt.wgsl
var<private> global: vec4<f32>;
var<private> global_1: vec4<f32>;

fn function() {
    let _e2 = global;
    global_1 = _e2;
    return;
}

@fragment 
fn fragment_doesnt(@location(0) param: vec4<f32>) -> @location(0) vec4<f32> {
    global = param;
    function();
    let _e3 = global_1;
    return _e3;
}

So AFAICT, this bug isn't reproduced by Rust-GPU main with default spirv-builder configuration (plus .multimodule(true)), target spirv-unknown-spv1.3 and either release or debug (by adding .release(false) to examples/multibuilder/src/main.rs, which produces the same results as the above).

Only other thing I can think of is --no-default-features --features use-installed-tools hiding some bug in SPIRV-Tools. I'll go toast my cores building C++ code and report back.

EDIT: absolutely no changes when I let it use SPIRV-Tools compiled by spirv-tools-sys from C++ source code.

@eddyb
Copy link
Collaborator

eddyb commented Dec 19, 2024

/// causes a warning by rust-gpu about having to be inlined anyway
#[inline(never)]

Btw, that's 100% expected. The user function is not the real entry-point, and the &mut used for the output (which we should replace with normal returns eventually) forces inlining.

@LegNeato
Copy link
Collaborator

Ok, so sounds like this might be a cago-gpu bug?

@eddyb
Copy link
Collaborator

eddyb commented Dec 19, 2024

Ok, so sounds like this might be a cargo-gpu bug?

Could be any of:

  • old Rust-GPU bug, now fixed
  • requires specific configuration to reproduce
  • cargo gpu moves some files around or something, breaking this?
    (I genuinely don't know how cargo gpu could cause this on its own)

@hadronized
Copy link

For what it’s worth, I came across the problem on this project. I’m keeping the branch that way until we can figure out what’s going on. And yes I’m using cargo gpu build.

@schell
Copy link
Contributor

schell commented Dec 19, 2024

Thanks for the link to your project @hadronized - I've recreated your problem and indeed, only one shader entry point is produced.

My guess is this is not cargo-gpu simply because compilation is happening. cargo-gpu just sets the stage for compilation to happen, then gathers the results, creates a manifest and puts it all in the output directory. Of course I could be wrong and something could be going wrong when doing that last step, but the fact that one entry point makes it all the way through the process makes me think there's something further up the tree that's the culprit.

I'll keep looking at this, thanks for the report @hadronized 👍 .

@schell
Copy link
Contributor

schell commented Dec 19, 2024

Hey! Look what I found in my own renderling codebase:

/// Simple fragment shader that writes the input color to the output color.
#[spirv(fragment)]
pub fn tutorial_passthru_fragment(in_color: Vec4, output: &mut Vec4) {
    use glam::Vec4Swizzles;
    // This seems silly, but without it rust-gpu doesn't emit a .spv file...
    *output = in_color.xyz().extend(in_color.w);
}

So this definitely pre-dates cargo-gpu.

@eddyb could it be that the entire entrypoint is getting optimized away in whatever parent function it's being inlined into, and so the entrypoint annotations (or whatever) disappear? Yup, already mentioned.

@eddyb
Copy link
Collaborator

eddyb commented Dec 19, 2024

@eddyb could it be that the entire entrypoint is getting optimized away in whatever parent function it's being inlined into, and so the entrypoint annotations (or whatever) disappear?

IMO that shouldn't be possible. The "user" function you're see is not the SPIR-V entry-point.

The entry-point function, as pointed to by OpEntryPoint (which is more like a special kind of export, than an annotation), is synthetic, and not at any risk from optimizations:

fn shader_entry_stub(
&self,
span: Span,
entry_func: SpirvValue,
entry_fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
hir_params: &[hir::Param<'tcx>],
name: String,
execution_model: ExecutionModel,
) -> Word {
let stub_fn = {
let void = SpirvType::Void.def(span, self);
let fn_void_void = SpirvType::Function {
return_type: void,
arguments: &[],
}
.def(span, self);
let mut emit = self.emit_global();
let id = emit
.begin_function(void, None, FunctionControl::NONE, fn_void_void)
.unwrap();
emit.end_function().unwrap();
id.with_type(fn_void_void)
};
let mut op_entry_point_interface_operands = vec![];
let mut bx = Builder::build(self, Builder::append_block(self, stub_fn, ""));
let mut call_args = vec![];
let mut decoration_locations = FxHashMap::default();
for (entry_arg_abi, hir_param) in entry_fn_abi.args.iter().zip(hir_params) {
bx.set_span(hir_param.span);
self.declare_shader_interface_for_param(
execution_model,
entry_arg_abi,
hir_param,
&mut op_entry_point_interface_operands,
&mut bx,
&mut call_args,
&mut decoration_locations,
);
}
bx.set_span(span);
bx.call(
entry_func.ty,
None,
Some(entry_fn_abi),
entry_func,
&call_args,
None,
None,
);
bx.ret_void();
let stub_fn_id = stub_fn.def_cx(self);
self.emit_global().entry_point(
execution_model,
stub_fn_id,
name,
op_entry_point_interface_operands,
);
stub_fn_id
}

You could even have the "user" functions (the ones on the Rust side, annotated with #[spirv(fragment)]/#[spirv(vertex)]/etc.) call eachother and I think that works: all of that happens at the level of functions that are themselves called from the real entry-points.


The much scarier thing is that not assigning the output might be UB?

I suppose e.g. SPIR-V -> WGSL might just read from an "uninitialized" variable, which, WGSL trying to be as safe as possible, is probably actually initialized under the hood (but I haven't actually checked any of this?)


So this definitely pre-dates cargo-gpu.

Can you reproduce anymore?

This produces a tutorial_passthru_fragment.spv (via multibuild) just fine, for me:

#![cfg_attr(target_arch = "spirv", no_std)]

use spirv_std::spirv;
use spirv_std::glam::Vec4;

/// Simple fragment shader that writes the input color to the output color.
#[spirv(fragment)]
pub fn tutorial_passthru_fragment(in_color: Vec4, output: &mut Vec4) {
}

And it even produces what we expected as far as WGSL is concerned:

$ naga target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/tutorial_passthru_fragment.{spv,wgsl}
$ cat target/spirv-unknown-spv1.3/release/deps/sky_shader.spvs/tutorial_passthru_fragment.wgsl
var<private> global: vec4<f32>;
var<private> global_1: vec4<f32>;

fn function() {
    return;
}

@fragment 
fn tutorial_passthru_fragment(@location(0) param: vec4<f32>) -> @location(0) vec4<f32> {
    global = param;
    function();
    let _e3 = global_1;
    return _e3;
}

If you're testing with cargo-gpu, make sure to try using the latest Rust-GPU main, it may be an old bug that got fixed without anyone realizing, for all we know.

@schell
Copy link
Contributor

schell commented Dec 19, 2024

Either way, cargo-gpu on main seems to be building a rustc_codegen_spirv.dylib that's out of step with what's passed in as the spirv-builder dep. That's definitely a bug. It seems it's always picking the rev 60dcb82, which is a couple months old. If this bug that @Firestar99 mentions was fixed has only landed in the past couple months then cargo-gpu could indeed be the problem, as it isn't grabbing the latest main from rust-gpu.

@schell
Copy link
Contributor

schell commented Dec 20, 2024

I'm now thinking this may be a difference in SpirvBuilder args.

@eddyb
Copy link
Collaborator

eddyb commented Dec 21, 2024

Oh, you default to spirv-unknown-vulkan1.2?
That's pretty significant, and might not be (as) compatible with WebGPU etc.
(Vulkan 1.0 or at most 1.1 is what I would consider the "baseline")

(click to open console log for spirv-unknown-vulkan1.2)
$ git show -s --oneline
f069c58c0c0 (HEAD, upstream/main, main) examples/runners/wgpu: avoid holding onto to multiple surfaces at the same time.
$ git diff examples/multibuilder
diff --git a/examples/multibuilder/src/main.rs b/examples/multibuilder/src/main.rs
index e694b72ee37..4572d9e02f8 100644
--- a/examples/multibuilder/src/main.rs
+++ b/examples/multibuilder/src/main.rs
@@ -3,7 +3,7 @@ use spirv_builder::{MetadataPrintout, SpirvBuilder};
 fn main() {
     let result = SpirvBuilder::new(
         concat!(env!("CARGO_MANIFEST_DIR"), "/../shaders/sky-shader"),
-        "spirv-unknown-spv1.3",
+        "spirv-unknown-vulkan1.2",
     )
     .print_metadata(MetadataPrintout::DependencyOnly)
     .multimodule(true)
$ cat examples/shaders/sky-shader/src/lib.rs
#![cfg_attr(target_arch = "spirv", no_std)]

use spirv_std::spirv;
use spirv_std::glam::Vec4;

#[spirv(fragment)]
pub fn fragment_works(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color +1.;
}

#[spirv(fragment)]
pub fn fragment_doesnt(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color;
}

/// causes a warning by rust-gpu about having to be inlined anyway
#[inline(never)]
#[spirv(fragment)]
pub fn fragment_works_again(v_color: Vec4, frag_color: &mut Vec4) {
  *frag_color = v_color;
}
$ cargo run -p multibuilder --release --no-default-features --features use-installed-tools 2>&1 | rg -U 'CompileResult \{[\s\S]+\}'
CompileResult {
    entry_points: [
        "fragment_doesnt",
        "fragment_works",
        "fragment_works_again",
    ],
    module: MultiModule(
        {
            "fragment_doesnt": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_doesnt.spv",
            "fragment_works": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_works.spv",
            "fragment_works_again": "/home/eddy/Projects/rust-gpu/target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_works_again.spv",
        },
    ),
}
$ echo target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/*.spv | xargs -t -n1 spirv-dis | rg OpEntryPoint
spirv-dis target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_doesnt.spv
               OpEntryPoint Fragment %1 "fragment_doesnt" %2 %3
spirv-dis target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_works.spv
               OpEntryPoint Fragment %1 "fragment_works" %2 %3
spirv-dis target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_works_again.spv
               OpEntryPoint Fragment %1 "fragment_works_again" %2 %3
$ cargo install naga-cli
    Updating crates.io index
     Ignored package `naga-cli v23.0.0` is already installed, use --force to override
$ naga target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_doesnt.{spv,wgsl}
$ cat target/spirv-unknown-vulkan1.2/release/deps/sky_shader.spvs/fragment_doesnt.wgsl
var<private> global: vec4<f32>;
var<private> global_1: vec4<f32>;

fn function() {
    let _e2 = global;
    global_1 = _e2;
    return;
}

@fragment
fn fragment_doesnt(@location(0) param: vec4<f32>) -> @location(0) vec4<f32> {
    global = param;
    function();
    let _e3 = global_1;
    return _e3;
}

So it still works on main just fine with spirv-unknown-vulkan1.2. I've also tried without --no-default-features --features use-installed-tools and no difference there.

I didn't see other SpirvBuilder arguments in cargo-gpu that could break anything significantly, so I'm guessing it's just the outdated backend?
(I've also not gotten around to reviewing cargo-gpu, and from my own experiments with making spirv-builder build rustc_codegen_spirv automagically, there are so many footguns and interactions that have to be accounted for, I dread the eventual months we'll need to spend on build system nonsense)


Also, feel free to try on different OSes - I remember rustup has different environment variable behavior on e.g. Windows, which made it hard to get it working uniformly etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants