batch mem2reg to process all variables in a single pass#547
batch mem2reg to process all variables in a single pass#547LegNeato wants to merge 3 commits intoRust-GPU:mainfrom
Conversation
|
@eddyb can you review this one? |
mem2reg processed each variable independently. This O(V*N) complexity caused multi-minute hangs on large post-inline functions. Batch all operations into single passes over the instructions. Fixes Rust-GPU#546. Measured on the proof-of-space-gpu shader: mem2reg drops from 228s to 18s (13x), total link from 236s to 26s. Also adds unit tests.
Firestar99
left a comment
There was a problem hiding this comment.
Looks sane to me, then again, I never looked at our mem2reg pass before. What's really interesting to me are the access patters of this pass, and how it reads and writes instructions, surely useful info for implementing some more efficient data structures for storing a module.
The SPIR-V spec allows OpLine/OpNoLine between OpPhi instructions at the start of a block. Account for this in the phi search so it doesn't stop early at a debug instruction. Adds two tests for the phi search boundary behavior.
Add doc comment to construct_access_chain_info explaining that access chain indices must be scalar integers per the SPIR-V spec, and that the constants map only tracks u32 (matching what rustc emits). Add test for a u64 constant index that is valid SPIR-V but not resolved by mem2reg, verifying the variable is not promoted.
|
As big improvement as it is, 18 seconds for |
|
Yeah, I didn't want to do any major changes and just moved multiple loops to batches. I can take a closer look after we get to a steady state. |
|
@Firestar99 @eddyb any stamp here? |
|
I'm getting back into catching up with Rust-GPU this week (ideally unblocking a release soon), and I'd want to wait for my own PR (last touched ~5 months ago) that both tracks the progress made on #63 and includes a couple of additional optimizations (mostly doing more More specifically, I would like to see the remaining improvement from this PR (after rebasing it on my branch), on the "worst offenders" shaders we have (my old benchmarks + this newer one) - there's a solid chance there will still be some improvement left, but I've been scared of soundness subtleties in the past. (The idea behind this PR seems fine, and the future SPIR-T "propagate values of locals" pass does handle all local variables together, instead of one at a time, but I haven't reviewed this yet) |
mem2reg processed each variable independently.
This O(V*N) complexity caused multi-minute hangs on large post-inline functions.
Batch all operations into single passes over the instructions.
Fixes #546. Measured on the proof-of-space-gpu shader: mem2reg drops from 228s to 18s, total link from 236s to 26s.
Also adds unit tests.
Disclosure: largely AI.