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

Open
wants to merge 4 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
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.

1 participant