Skip to content

print: name and reuse inlined call site debuginfo "scopes". #18

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
Jul 1, 2025

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Jun 30, 2025

This addresses the visual (and functional) bloat caused by accurate (wrt inlining) debuginfo from Rust-GPU,
which on the motivating example, results in these changes:

(middle column obtained by making DbgScope::maybe_print_name_as_ref_or_def always return None, which should not be considered likely output after this PR, it's only kept around as a fallback, e.g. if intra-function debuginfo ends up used in Const attributes etc.)

Before this PR New style w/o reuse After this PR
.spirt
line count
190498 190498 46365
.spirt.html
file size
96 MiB 95.2 MiB 33.5 MiB
Worst-case
comparison

(EDIT: I've sadly had to force widths via explicit <img> tags because GitHub started applying overflow-wrap: anywhere to the entire markdown render output, which means individual letters in the text cells are competing against whole <img>s, during table layout, and the letters end up inevitably all in one vertical line)


Initially I was hoping for some kind of "stateful" comment system, and/or showing "debug scopes" using braces within the debuginfo comments, but the // d123 = ... notation added in this PR was an algorithmic nightmare as-is. (mainly because of multi-version support, where uses of debuginfo can easily disappear/move as nodes get simplified/wholly removed/etc. in various passes, making it hard so share common placements for such debuginfo "definitions")

Also, it's been aesthetically tricky to handle the transition from "location of (inlined) call site" to "location inside (inlined) callee", and this PR ended up flipping the order, from inside-out to outside-in. (you might be able to tell between the first and second column, it used to look like a backtrace that keeps "zooming out", from leaf function to entry-point, whereas now it "zooms in", keeping the leaf-most location closest to whatever follows the debuginfo comments)

That 33.5 MiB .spirt.html after this PR still makes browsers a bit unhappy. (I was going to share more comparisons but even that single one took minutes per screenshot just to scroll with the Firefox screenshot tool!)
(specific HTML size/complexity issues might be fixable, but it's hard to tell what browsers are struggling with most, without actually testing a tentative change)

However, it's a far cry from what it used to be, and any visual and UX improvements are welcome.
(let alone the 216 MiB file I started from, before fixing the format_args! "decompiler" on my Rust-GPU branch, as the output was full of core::fmt internals, esp. kept alive by function pointers/vtables)


Not sure how to handle reviews, but I would gladly walk someone through all of this mess if anyone cares.
(Thought, just like the other recent pretty-printer PRs, this is purely about aesthetics/ergonomics, so anything being "wrong" or "imprecise" shouldn't affect the correctness of actual SPIR-T passes etc.)

@eddyb eddyb requested a review from a team June 30, 2025 23:51
@eddyb eddyb enabled auto-merge July 1, 2025 00:31
Copy link
Member

@Firestar99 Firestar99 left a comment

Choose a reason for hiding this comment

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

I feel like as this is just debug print, and it massively helps you, we should just merge it without me explicitly reviewing every single part of it

@eddyb eddyb added this pull request to the merge queue Jul 1, 2025
Merged via the queue into Rust-GPU:main with commit 1db5c95 Jul 1, 2025
6 checks passed
@eddyb eddyb deleted the print-dbg-scope branch July 1, 2025 08:07
github-merge-queue bot pushed a commit that referenced this pull request Jul 1, 2025
…dren. (#19)

Follow-up to:
- #18

Overlooked an edge case (`scope.parent()` being in the `claimed` map,
but at a later position than `scope` itself, i.e. part of the same
"claim" batch) in the original PR, now the order is properly
"outside-in":
|Before|After|
|-|-|

|![image](https://github.com/user-attachments/assets/e80ddfc8-62d7-4e6a-8b98-5c791bbed66b)|![image](https://github.com/user-attachments/assets/45476c72-1790-482e-a1fd-b3ea7443f4ac)|
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