Skip to content

Try dumping, when a SPIR-T pass panics, its in-progress spirt::Module state. #362

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

Merged
merged 1 commit into from
Aug 14, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Aug 11, 2025

While SPIR-T passes should, in principle, never panic, it can still happen (esp. with WIP code, where e.g. assert!s or todo!()s might be overrepresented etc.), and before this PR, the partially modified SPIR-T module would be completely lost in the --dump-spirt-passes output.

And because incomplete SPIR-T mutation could result in extremely malformed modules, this PR also wraps the SPIR-T pretty-printing in std::panic::catch_unwind, removing one pass' "after" state, at a time, from the pretty-printed versions, while it still panics, and showing an extra warning (with all the pass names that couldn't be printed) if that was necessary at all.

Also, there's a minor ergonomic fix in that after_pass doesn't need to also be passed the same string as before_pass (since SpirtDumpGuard is tracking it anyway).

@eddyb eddyb enabled auto-merge August 11, 2025 23:03
Copy link
Collaborator

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

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

This looks ok, but I can't help but think this should be tracing spans and items. Then you would get nesting and such for free.

@eddyb
Copy link
Collaborator Author

eddyb commented Aug 14, 2025

I can't help but think this should be tracing spans and items

That would be useful for some stuff, but, even ignoring the rustc "timers"...
(that we only use an inefficient ad-hoc form of, rn, which is only accessible via RUSTGPU_RUSTFLAGS=-Ztime-passes, while the more advanced measureme APIs are the low-overhead/high-throughput event tracking powering -Zself-profile, which the log/tracing approach can't really ever compete with AFAIK, sadly - but really, few things need that level of profiling, compilers being deterministic and IO-bound is an edge case)
the most important part here is generating the .spirt/.spirt.html dumps, where the "pass names" show up as table headings - quick example from a dump I have open:
image

I wouldn't mind automating those strings, but right now they're quite arbitrary, e.g. after Rust-GPU/spirt#25 the mem::analyze_accesses one comes from:

let profiler = before_pass("mem::analyze_accesses", module);
spirt::passes::qptr::analyze_mem_accesses(module, &SPIRT_MEM_LAYOUT_CONFIG);
after_pass(Some(module), profiler);

with spirt::passes::qptr::analyze_mem_accesses defined as:

// FIXME(eddyb) this doesn't really belong in `qptr`.
pub fn analyze_mem_accesses(module: &mut Module, layout_config: &crate::mem::LayoutConfig) {
    crate::mem::analyze::GatherAccesses::new(module.cx(), layout_config)
        .gather_accesses_in_module(module);
}

So there's not even any function named analyze_accesses in a module named mem, at best that could be the case for spirt::passes::mem::analyze_accesses if that function was moved to such a place.

But even if we take the function's path and strip either spirt::passes:: or rustc_codegen_spirv::linker::spirt_passes:: as a prefix (which does sound relatively reasonable, I will admit), how could we obtain that information when that function is in another crate?

@LegNeato
Copy link
Collaborator

LegNeato commented Aug 14, 2025

You can create trace spans programmatically with custom names, it doesn't have to be a function nor a proc macro. Tracing is in rustc proper and lots of crates (not sure what is actually instrumented though).

But yeah, it doesn't help with table headings unless you installed your own subscriber and collected and then output (that might actually be interesting).

@eddyb eddyb added this pull request to the merge queue Aug 14, 2025
Merged via the queue into Rust-GPU:main with commit 72943b5 Aug 14, 2025
13 checks passed
@eddyb eddyb deleted the dump-wip-on-panic branch August 14, 2025 14:18
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.

2 participants