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

SPIR-T panics with assertion failed: list_eq_key(children) ==\n list_eq_key(self.func_def_body.at(unstructured_region).def().children). #1086

Open
schell opened this issue Jul 28, 2023 · 4 comments
Assignees
Labels
t: bug Something isn't working

Comments

@schell
Copy link

schell commented Jul 28, 2023

While debugging an issue with nested loops I simplified my shader to one loop as a sanity check and then hit this compiler panic. It seems to be caused by looping indefinitely (in my shader I forgot to increment a value used in the break condition).

Expected Behaviour

Compiler should error instead of panic.

Actual Behavior

Compiler panics with:

thread 'rustc' panicked at 'assertion failed: list_eq_key(children) ==\n    list_eq_key(self.func_def_body.at(unstructured_region).def().children)',
  /Users/schell/.cargo/registry/src/index.crates.io-6f17d22bba15001f/spirt-0.2.0/src/cfg.rs:1214:13

Example & Steps To Reproduce

Compile this shader:

use glam::{Vec3, Vec4, Vec4Swizzles, Vec3Swizzles};
use spirv_std::{image::Cubemap, Sampler, spirv};

#[cfg(target_arch = "spirv")]
use spirv_std::num_traits::Float;

#[spirv(fragment)]
pub fn fragment_convolve_diffuse_irradiance(
    environment_texture: &Cubemap,
    sampler: &Sampler,
    local_pos: Vec3,
    frag_color: &mut Vec4,
) {
    let pi: f32 = 3.1415927;

    let normal = local_pos.normalize_or_zero();
    let mut irradiance = Vec3::new(0.0, 0.0, 0.0);
    let right = Vec3::new(0.0, 1.0, 0.0).cross(normal).normalize_or_zero();
    let up = normal.cross(right).normalize_or_zero();

    let nr_samples = 0.0;
    irradiance += environment_texture.sample(*sampler, up).xyz();
    loop {
        if nr_samples > 2.0 {
            break;
        }

        irradiance += environment_texture.sample(*sampler, up).xyz();
    }
    let color = irradiance * (pi / nr_samples);
    *frag_color = color.xyz().extend(1.0);
}

System Info

note: rustc 1.70.0-nightly (84dd17b56 2023-04-14) running on aarch64-apple-darwin

note: compiler flags: --crate-type dylib -C opt-level=3 -C embed-bitcode=no -C codegen-units=1 -Z unstable-options -Z codegen-backend=/Users/schell/code/renderling/shaders/target/release/librustc_codegen_spirv.dylib -Z binary-dep-depinfo -C symbol-mangling-version=v0 -Z crate-attr=feature(register_tool) -Z crate-attr=register_tool(rust_gpu) -C llvm-args=--module-output=multiple

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack
note: rust-gpu version 0.8.0

  • Rust: [e.g. 1.49.0-nightly (1eaadebb3 2020-10-21)]
  • OS: [e.g. macOS 10.15.7]
  • GPU: [e.g. Intel(R) UHD Graphics 630]
  • SPIR-V: [e.g. v2020.3 unknown hash, 2020-06-12T01:06:18]

Backtrace

Backtrace

stack backtrace:
   0:        0x102eb5894 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::hec12b20601f2bdf0
   1:        0x102f089a0 - core::fmt::write::h84e52a09c8a95be7
   2:        0x102eab344 - std::io::Write::write_fmt::h694b0891aa3b64d5
   3:        0x102eb56a8 - std::sys_common::backtrace::print::hdefc03f01b0d83f1
   4:        0x102eb80bc - std::panicking::default_hook::{{closure}}::hc4c0bdc7bd459aa9
   5:        0x102eb7e7c - std::panicking::default_hook::h6dc5d3ac0202ebe6
   6:        0x10470d6f4 - rustc_codegen_spirv::__rustc_codegen_backend::{{closure}}::h6edd25957ff152e7
   7:        0x102eb877c - std::panicking::rust_panic_with_hook::h04718b2e9bd107f5
   8:        0x102eb8534 - std::panicking::begin_panic_handler::{{closure}}::h1c251c8e7c7e5969
   9:        0x102eb5cb4 - std::sys_common::backtrace::__rust_end_short_backtrace::hc2f6697e294cde53
  10:        0x102eb8308 - _rust_begin_unwind
  11:        0x102f34420 - core::panicking::panic_fmt::hcabee496664f2172
  12:        0x102f34490 - core::panicking::panic::hc56ef5c96c7e745b
  13:        0x1049027e8 - spirt::cfg::Structurizer::structurize_func::h8ccbca14134c4bed
  14:        0x10488ac74 - spirt::passes::legalize::structurize_func_cfgs::h5a9bb2f8c14010b3
  15:        0x1046b6974 - rustc_codegen_spirv::linker::link::h666c87acc0f36829
  16:        0x10465bae0 - rustc_codegen_spirv::link::link::h87c14272e01e3168
  17:        0x10470bf54 - <rustc_codegen_spirv::SpirvCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::link::ha3f63258b51407e5
  18:        0x10b47f7f8 - <rustc_interface[b2f1c94be2f1d7cc]::queries::Linker>::link
  19:        0x10b3b3da4 - rustc_span[dc18a910ba5122f1]::set_source_map::<core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>, rustc_interface[b2f1c94be2f1d7cc]::interface::run_compiler<core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>, rustc_driver_impl[27bd541b75fbd8b5]::run_compiler::{closure#1}>::{closure#0}::{closure#0}>
  20:        0x10b3c7184 - std[b04369226ecd3a5b]::sys_common::backtrace::__rust_begin_short_backtrace::<rustc_interface[b2f1c94be2f1d7cc]::util::run_in_thread_pool_with_globals<rustc_interface[b2f1c94be2f1d7cc]::interface::run_compiler<core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>, rustc_driver_impl[27bd541b75fbd8b5]::run_compiler::{closure#1}>::{closure#0}, core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>>
  21:        0x10b3b8364 - <<std[b04369226ecd3a5b]::thread::Builder>::spawn_unchecked_<rustc_interface[b2f1c94be2f1d7cc]::util::run_in_thread_pool_with_globals<rustc_interface[b2f1c94be2f1d7cc]::interface::run_compiler<core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>, rustc_driver_impl[27bd541b75fbd8b5]::run_compiler::{closure#1}>::{closure#0}, core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[723a1ab69fb7b501]::result::Result<(), rustc_span[dc18a910ba5122f1]::ErrorGuaranteed>>::{closure#1} as core[723a1ab69fb7b501]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}
  22:        0x102ec0e24 - std::sys::unix::thread::Thread::new::thread_start::h5343322b5f9311e3
  23:        0x18df97fa8 - __pthread_joiner_wake

@schell schell added the t: bug Something isn't working label Jul 28, 2023
@schell schell changed the title Compiler panic w/ loop Compiler panic w/ endless loop Jul 28, 2023
@eddyb eddyb changed the title Compiler panic w/ endless loop SPIR-T panics with assertion failed: list_eq_key(children) ==\n list_eq_key(self.func_def_body.at(unstructured_region).def().children). Jul 28, 2023
@eddyb
Copy link
Contributor

eddyb commented Jul 28, 2023

I've edited the description and title to explicitly mention the message.

This is a "known" panic I have yet to investigate because so far I've only hit it with RUSTGPU_CODEGEN_ARGS=--no-early-report-zombies, which can let some malformed SPIR-V rustc_codegen_spirv generates through, so I just assumed that was the cause here.

If it can happen without RUSTGPU_CODEGEN_ARGS, then this deserves a lot more scrutiny. I'll attempt to minimize the testcase provided (is it complete though? there's no #[spirv(...)], so that's just dead code? EDIT: was just a missing #[spirv(fragment)], added that to the issue description, can confirm it repros).

@eddyb eddyb self-assigned this Jul 28, 2023
@eddyb
Copy link
Contributor

eddyb commented Jul 28, 2023

This reduces somewhat bizarrely:

use spirv_std::spirv;

#[inline(never)]
fn bug(cond: bool) {
    if (if cond { true } else { false }) {}
    loop {}
}

#[spirv(fragment)]
pub fn main() {
    bug(false);
}

I'll have to move --dump-spirt-passes into a helper type that implements Drop so panics don't stop it from showing useful information (in general it seems sensible to do that sort of thing, just because removing 100% of possible panics is hard, esp. before we have a SPIR-T validator).

@eddyb
Copy link
Contributor

eddyb commented Aug 1, 2023

Starting to suspect it's in part caused by this structurizer special-casing, which has a FIXME:

            // Structured return, the function is fully structurized.
            //
            // FIXME(eddyb) also support structured return when the whole body
            // is divergent, by generating undef constants (needs access to the
            // whole `FuncDecl`, not just `FuncDefBody`, to get the right types).

I forgot about this since, because it apparently never(?) caused any issues in practice, but e.g. "always reach an infinite loop" Rust-GPU shaders wouldn't end up 100% structured - my best guess for why spirv-val doesn't complain is that the actual body is structured, there's just a single unstructured branch at the very start, which SPIR-V doesn't care about ("structured SPIR-V" only puts requirements on merging selects/loops).

github-merge-queue bot pushed a commit to EmbarkStudios/spirt that referenced this issue Jan 31, 2024
…turization. (#55)

Original motivation back when I first started this refactor was this
bug:
- EmbarkStudios/rust-gpu#1086

Progress stalled back then and I only revived the branch for the Vulkan
layer (#53), because trying
to run the Vulkan CTS with the SPIR-T layer would run into:
- missing `OpSpecConstantOp` support (already implemented in the Vulkan
layer branch)
- structurizer region desync `list_eq_key` panics (fixed by this PR)
- missing `OpTypeForwardPointer` support (not implemented yet, but it's
much rarer and only used with `PhysicalStorageBuffer` pointers, since
logical pointers don't allow recursive data types in the first place)

On top of unblocking the SPIR-T Vulkan layer for in-depth testing, this
refactor, out of necessity of not introducing even more spurious unused
`bool`s, makes the existing ones as lazy as possible (within the limits
of what the structurizer knows without adding more analysis passes).

You can see in the `README.md` example, that this PR removes *both*
extraneous `bool`s (for "does `break`" vs "doesn't `break`"), and
instead the original condition (`v2`) is now used directly by the loop.

I've also theoretically come up with even more advanced techniques (such
as taking N disjoint targets that can be dispatched all at once, and
lazily generating the appropriate propagation of integer values `0..N`
to use a `switch`), but they either require other PRs to land as well
first, and/or are too invasive and this has already been a ridiculous
sunken cost compared to the measely benefits it offers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants
@schell @eddyb and others