Skip to content

Make SpirvValue(Kind) representation and operations more orthogonal and robust (lossless). #348

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

eddyb
Copy link
Collaborator

@eddyb eddyb commented Jul 24, 2025

SpirvValue, and especially SpirvValueKind, have been serving several purposes so far:

  • tracking the type (i.e. struct SpirvValue { kind: SpirvValueKind, ty: ... } before this PR)
  • encoding values that we can't (or just don't, currently) emit any SPIR-V for
    • ideally should become SpirvConsts (already have some of that done for later PRs)
  • indicating that a value is illegal and therefore needs a "zombie" (deferred error) applied
    • in the case of SpirvValueKind::IllegalConst, a Span isn't known initially at all
      (so there's a need to defer until the time of SpirvValue::def, which may bring a Span)
  • tracking enough information to let SpirvValue::strip_ptrcasts undo pointer casts
    • i.e. SpirvValueKind::LogicalPtrCast contains the value and type IDs for a SpirvValue
      (SpirvValue { kind: SpirvValueKind::Def(original_ptr), ty: original_ptr_ty })

That has grown ad-hoc, and it can be a nightmare to extend any part of it, hence this work.

More specifically, this PR:

  • emits all "zombie" (deferred error) messages early, and only defers the Span at most
    • a new bool field (zombie_waiting_for_span) replaces SpirvValueKind::IllegalConst
    • as a result, SpirvValue::def (which is called everywhere to obtain a SPIR-V ID)
      only has to check a bool (for registering the Span), no more string formatting
    • the only reason this might not be an overall perf win, is that pointer casts now have
      to generate the string message themselves, but it's unclear if that was ever skipped
      (and I wasn't able to observe any change in build times, tho more testing is welcome)
  • replaces SpirvValueKind::LogicalPtrCast with an equivalent Option in SpirvValueKind::Def
    • by making SpirvValue generic and reusing it, SpirvValue::strip_ptrcasts becomes
      simpler and combining it with pointercast is now lossless (wrt other details)
    • in theory, this can now allow a SpirvValue that is simultaneously like the old
      SpirvValueKind::IllegalConst and SpirvValueKind::LogicalPtrCast at the same time
      (arguably the main motivation for this PR, specifically as a const_bitcast result)
  • keeps whole SpirvValues in the SpirvConst<->SPIR-V ID "const interning" maps
    • while more wasteful space-wise (not even using SpirvValue's new generic param),
      this removes all duplicated logic around "deriving SpirvValue for a constant"
      (i.e. what used to decide whether SpirvValueKind::IllegalConst should be used)
    • SpirvValue::const_fold_load becomes trivial, but even more important, lossless
      (returning the same exact SpirvValue as the original SpirvConst did)

@eddyb eddyb force-pushed the robust-spirv-value-zombies branch from 1e33b18 to b4ed844 Compare July 26, 2025 12:49
@eddyb eddyb marked this pull request as ready for review July 26, 2025 12:50
@eddyb eddyb enabled auto-merge July 26, 2025 14:42
@Firestar99
Copy link
Member

Firestar99 commented Jul 28, 2025

Tested on my nanite-at-home project, 3 runs, first run after branch switch discarded (due to many CPU crates recompiling), cmdline:
rm -rf ./target/spirv-builder/release/ ./target/spirv-builder/spirv-unknown-vulkan1.2/; RUSTGPU_CARGOFLAGS=--timings cargo b --release

older master 8ee9f2f: 16.3s 16.4s 16.6s
current master 87ea628: 15.9s 15.9s 15.9s (primarily #350)
this branch b4ed844: 15.7s 15.8s 15.9s

This branch is slightly faster 🎉

@eddyb eddyb marked this pull request as draft July 28, 2025 22:48
auto-merge was automatically disabled July 28, 2025 22:48

Pull request was converted to draft

@eddyb
Copy link
Collaborator Author

eddyb commented Jul 28, 2025

This branch is slightly faster 🎉

Wow, that's in the opposite direction from what I heard from @nazar-pc, but it's good to know it can swing either way.


In the meanwhile, I really want to land certain soundness fixes in read_from_const_alloc, which were quite indirectly (const_bitcast of constant pointers - pun unintended) blocked on this, except not really.

So I've marked this PR as draft, and I hope to come up with something isolated, and we can land this later.

EDIT: the soundness fix PR is now up:

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