search

KhronosGroup / SPIRV-Tools

Apache License 2.0
1.05k stars 547 forks source link

Implicit LOD functions can be supported when compute shader derivatives are supported #5674

Open EpicJeanNoeMorissette opened 3 months ago

EpicJeanNoeMorissette commented 3 months ago

https://github.com/KhronosGroup/SPIRV-Tools/blob/ccf3e3c1035189bbb50793d1e249a2c0ba3388a3/source/opt/replace_invalid_opc.cpp#L84

When using DXC, the SPIRV is correctly generated prior: the proper extensions (SPV_NV_compute_shader_derivatives) and execution mode (DerivativeGroupLinearNV or Quad) are specified, but then this pass goes through and removes all the instructions.

We simply added "model != spv::ExecutionModel::GLCompute" in our local fork, but it would probably be cleaner to also ensure that the extension is active and rename the function.

s-perron commented 3 months ago

I remember adding this pass. I forget the details, but I sort-of regret adding the pass. Before I update it, I'd like to revisit if it is really needed. Do you have a sample that you can share where an instruction must be removed to be valid? I'd like to understand why the instruction is not dead. It scares me a little that we simply replace an instruction that is still used. If needed, I will update the pass.

EpicJeanNoeMorissette commented 3 months ago

To be honest, I feel exactly the same way you do: I was surprised to see this pass simply remove instructions that were in use (although there were no side effects for what it's worth). I would sort of expect a compilation error with a detailed message (this only gave me a warning which I didn't notice at first).

s-perron commented 3 months ago

I would sort of expect a compilation error with a detailed message (this only gave me a warning which I didn't notice at first).

If you disable the pass in your workflow, you will get a validation error in the shaders that contain the invalid instructions. I'll close the issue for now. Reopen if you decide you need the pass updated.

EpicJeanNoeMorissette commented 3 months ago

Reopen if you decide you need the pass updated.

I'm not sure I understand? :) Left unchanged, this pass removes all implicit LOD functions from my compute shaders when using compute shader derivatives. This breaks my compute shaders, that much I am certain about.

s-perron commented 3 months ago

What happens if you do not use the pass at all for any shaders? That is what I wanted to know. It is possible the pass is no longer needed. If it is still needed, can we improve optimizations so that we can deprecate the pass. I believe you are the only users.

EpicJeanNoeMorissette commented 3 months ago

I'll give it a try and report back!