Skip to content

fix(naga spv-out): OpConstantComposite does not support flattening#9940

Draft
andyleiserson wants to merge 2 commits into
gfx-rs:trunkfrom
andyleiserson:jj-push-kzwn
Draft

fix(naga spv-out): OpConstantComposite does not support flattening#9940
andyleiserson wants to merge 2 commits into
gfx-rs:trunkfrom
andyleiserson:jj-push-kzwn

Conversation

@andyleiserson

@andyleiserson andyleiserson commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

SPIR-V's OpConstantComposite does not support flattening vector constructors like vec4(vec2(), 1, 1).

flatten_compose unfortunately can't handle these for us, because (unlike a nested composite or splat) the component zero value may not be in the arena. So add a dedicated helper in the SPIR-V backend to handle this.

The constant evaluator does have a function eval_zero_value_and_splat, which is used when this kind of nested compose appears in a larger const-evaluatable context. But in the specific case here, the nested compose appeared as an argument to a non-const function call. The constant evaluator didn't trigger on that.

Even if the constant evaluator did resolve this, unless we declare that it is invalid IR, the SPIR-V backend still also needs to be able handle it correctly.

Fix for Firefox bug https://bugzilla.mozilla.org/show_bug.cgi?id=1984765.

Testing
Adds snapshot tests.

Squash or Rebase? Squash

Checklist

  • I self-reviewed and fully understand this PR.
  • WebGPU implementations built with wgpu may be affected behaviorally.
  • Validation and feature gates are in place to confine behavioral changes.
  • Tests demonstrate the validation and altered logic works.
  • CHANGELOG.md entries for the user-facing effects of this change are present.
    • Needed
  • The PR is minimal, and doesn't make sense to land as multiple PRs.
  • Commits are logically scoped and individually reviewable.
  • The PR description has enough context to understand the motivation and solution implemented.

@jimblandy jimblandy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The whole idea of naga::proc::flatten_compose is to produce an iterator over actual components, not vector-typed ZeroValue expressions. So I think we have more bugs than this; I would expect most uses of flatten_compose to be wrong.

I think rather than adding a new function that finishes the job that flatten_compose left undone, we need to change flatten_compose to return some indication of "this component is zero".

@jimblandy

Copy link
Copy Markdown
Member

Like, having flatten_compose return Option<Handle<Expression>> would be the cheapest way to do it. That seems a little obscure, though, so maybe we could add a new enum:

enum FlattenedComponent {
    Zero,
    Expression(Handle<Expression>),
}

and have flatten_compose return impl Iterator<Item = FlattenedComponent>.

And then we'd need to see what the call sites look like.

@andyleiserson

andyleiserson commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Many places that flatten_compose is called, eval_zero_value_and_splat is called just prior. The exceptions are the two in the SPIR-V backend in this PR, and:

ConstantEvaluator::swizzle:

crate::proc::flatten_compose(ty, components, self.expressions, self.types)

ConstantEvaluator::access:

crate::proc::flatten_compose(ty, components, self.expressions, self.types)

Seems like these two are just broken (sorry to paste as image):

@andyleiserson
andyleiserson marked this pull request as draft July 24, 2026 17:07
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