(Naga) Implement OpSpecConstantOp for the SPIR-V frontend

[hasenbanck]

Connections

#4324

Description

Implement OpSpecConstantOp for the SPIR-V frontend.

This implementation solves all my problems that I had with SLANGC output.
But I couldn't implement all operations. The following are missing:

• Op::VectorShuffle, Op::CompositeExtract, Op::CompositeInsert => All work on non-scalars, which I can't get to work.
• Op::QuantizeToF16 => We don't support QuantizeToF16 in the SPIR-V frontend yet. I think the constant evaluator might also have problems with it.

We also properly convert implicit type conversions to explicit type conversions, convert signed shifts to unsigned shifts and also optimize common type conversion patterns produced by the GLSL compiler or SLANG.

Testing

I added two snapshot tests. One based on GLSL code and one in pure SPVASM, that tests more complex chained OpSpecConstantOps.

Squash or Rebase?

Rebase

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy --tests. If applicable, add:
• Run cargo xtask test to run tests.
• If this contains user-facing changes, add a CHANGELOG.md entry.

[hasenbanck]

I added a second commit to the PR that removes a workaround and instead uses the same approach as the GLSL frontend, by properly using the ConstantEvaluatorto evaluate the constant expressions. This is a more invasive change, so I decide it to add as a second commit, but could be spun into it's own PR if the reviewer think that's the better option.

This would enable us in a future PR to extend the ConstantEvaluator to have a mode for SPIR-V, that takes care for the corner cases that compiler could create (like the corner cases by the GLSL compiler described above).

[hasenbanck]

Since no reviewer has assigned the PR to them, I took the liberty and added a third commit that polishes the implementation:

Implicit type conversions are now made explicit, so that we can now properly convert OpSpecConstantOp created by GLSL, that contain implicit type conversion properly into stricter forms, so that WGSL can validate and evaluate the overrides correctly.

We now also properly use the best available name for the override and also detect and optimize type conversion patterns that compiler like GLSL love to create. This improves debuggability of the produced shaders greatly.

I will update the PR text accordingly.

[hasenbanck]

Hi! It has been 4 weeks since I pushed this PR. Is there any way I could help get this PR faster through the pipeline?

[inner-daemons]

Just wanted to say, thanks for putting up this PR and thanks for your patience! This seems very valuable to have. It has been reassigned so hopefully it will get in soon.

[teoxoy]

The implementation looks good overall but I have 2 main concerns:

• We now have 2 places where we have to handle the same operation type and make sure that their semantics get translated to our IR's semantics in the same way. Please try to refactor the code to make both code-paths use the same logic.
• About the try_optimize_x functions; naga is not an optimizing compiler and we don't usually have such logic. Also, since these expressions will be processed by the constant evaluator they will get handled anyway.

[teoxoy]

Actually, is it worth trying to eval here? Since we are dealing with OpSpecX I would imagine the input to not contain fully const expressions.

[hasenbanck]

I tried skipping evaluation entirely and just appending expressions directly, but ran into validation failures because the expression kind tracking relies on the evaluator's logic to correctly propagate Override vs Const status through chained expressions. Because of this I couldn't get it to work without try_eval_and_append().

[teoxoy]

The whole expression tree shouldn't get marked as const. It should be ok to have override expressions contain const expressions. If that's not the case it's a bug. We use .to_expr() in existing code:

wgpu/naga/src/front/spv/mod.rs

Lines 5852 to 5855 in ed6b789

	let constant = self.lookup_constant.lookup(component_id)?;
	let expr = module
	.global_expressions
	.append(constant.inner.to_expr(), span);

[hasenbanck]

This was fixed and removed in the "(Naga) Remove override workaround and properly track expression kind in the SPIR-V frontend" commit.

[hasenbanck]

Thanks for the review! I removed the try_optimize_x() and answered your comments in the code.

But I don't properly understand this point:

We now have 2 places where we have to handle the same operation type and make sure that their semantics get translated to our IR's semantics in the same way. Please try to refactor the code to make both code-paths use the same logic.

What do you mean by this? Currently we have two code paths next_block() and parse_spec_constant_op() that convert the sematics in very different contexts. Merging their behavior would complicate the code in my eyes and I don't think that I would be the right person to do so. If you want to have them merged, then I kindly ask you to take over that task, since you have a clearer vision on how you want to have the code structured.

[inner-daemons]

@hasenbanck Can you resolve the conflicts and merge in the latest commits from trunk? The PR is quite old at this point and you will likely have to make a few tweaks.

[hasenbanck]

@hasenbanck Can you resolve the conflicts and merge in the latest commits from trunk? The PR is quite old at this point and you will likely have to make a few tweaks.

Done.