Emit execution mode of type per entry point once. Emit SPIRV capability once per shader program. #4189
Conversation
Added a dictionary<SpvWord,Hash<ExecutionMode>> to ensure we don't emit multiple.
source/slang/slang-emit-spirv.cpp
Outdated
| @@ -2765,14 +2765,14 @@ struct SPIRVEmitContext | |||
| if (isQuad) | |||
| { | |||
| verifyComputeDerivativeGroupModifiers(this->m_sink, inst->sourceLoc, true, false, numThreadsDecor); | |||
| emitOpExecutionMode(getSection(SpvLogicalSectionID::ExecutionModes), nullptr, entryPoint, SpvExecutionModeDerivativeGroupQuadsNV); | |||
| emitOpCapability(getSection(SpvLogicalSectionID::Capabilities), nullptr, SpvCapabilityComputeDerivativeGroupQuadsNV); | |||
| requireSPIRVExecutionMode(nullptr, getID(ensureInst(entryPoint)), SpvExecutionModeDerivativeGroupQuadsNV); | |||
There was a problem hiding this comment.
Can we just do getID(entryPoint)?
There was a problem hiding this comment.
I can make it so getID(IRInst) calls getID(ensureInst(entryPoint))?
There was a problem hiding this comment.
Just use getIRInstSpvID.
source/slang/slang-emit-spirv.cpp
Outdated
| emitOpExecutionMode( | ||
| getSection(SpvLogicalSectionID::ExecutionModes), | ||
| parentInst, | ||
| entryPoint, | ||
| executionMode, | ||
| ops... | ||
| ); |
There was a problem hiding this comment.
I think you can replace this lines to
emitInst(getSection(SpvLogicalSectionID::ExecutionModes),
parentInst,
SpvOpExecutionMode,
entryPoint,
executionMode,
ops...);
and remove the function emitOpExecutionMode().
Because the function is used only once here.
And the body is not doing anything much.
There was a problem hiding this comment.
I can remove it for now.
If someone else has a reason for the design to keep the wrapper, it should be trivial to add back anyways.
| // CHECK_SPV_QUAD_C-COUNT-1: DerivativeGroupQuadsNV | ||
| // CHECK_SPV_QUAD_C-COUNT-1: "SPV_NV_compute_shader_derivatives" |
There was a problem hiding this comment.
I don't think -COUNT-1 will do what you want.
You will need to do,
// CHECK_SPV_QUAD_C: DerivativeGroupQuadsNV
// CHECK_SPV_QUAD_C-NOT: DerivativeGroupQuadsNV
// CHECK_SPV_QUAD_C: "SPV_NV_compute_shader_derivatives"
// CHECK_SPV_QUAD_C-NOT: "SPV_NV_compute_shader_derivatives"
There was a problem hiding this comment.
I checked-- You are correct.
Thank-you for catching this.
Added a dictionary<SpvWord,Hash> && wrapper to function like requireSPIRVCapabilities to ensure we don't emit multiple execution modes. This wrapper is called
requireSPIRVExecutionMode.Change a method of adding a SPIRV capability to use
requireSPIRVCapabilitiesto be more correct (only emit 1 capability op).fixes: #4185