Allows Vulkan spec constants as attribute arguments. #7439
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
danbrown-amd
force-pushed
the
attr-arg-consts
branch
3 times, most recently
from
9db3b6c to
243acbc
Compare
There was a problem hiding this comment.
I don't mind this change generally. It will be good for SPIR-V.
I don't know if this is complete yet (it is still a draft). I pointed out one issue with using LocalSize. You might also need to look at SpirvEmitter::addDerivativeGroupExecutionMode(). That function determines which derivative group execution mode to add based on the number of threas in the group. However, if you have a spec constant for the number of threads, we cannot do that.
danbrown-amd
force-pushed
the
attr-arg-consts
branch
2 times, most recently
from
58352af to
357813d
Compare
danbrown-amd
force-pushed
the
attr-arg-consts
branch
from
357813d to
8e898bf
Compare
|
Why was Of course, since there's no access to specializations at compile time, there's no way to ensure the best choice of execution mode with spec constants anyway. However, in some cases partial information would be sufficient to rule out one of them. If both are possible, maybe pick one and issue a warning? |
danbrown-amd
changed the title
It was implemented that way because that was the HLSL spec implemented for DXIL. See Thread Groups and Quads. I don't mind if we diverge from it when there are specialization constants. We just need to document it.
That is a reasonable idea. Longer term, we could try to add explicit attribute that apply to the entry point that would override the guess. |
Also restores file inadvertently committed source file changed for debugging.
| // Note: validation disabled until NodePayloadAMDX pointers are allowed | ||
| // as function arguments |
There was a problem hiding this comment.
Should these comments be removed?
There was a problem hiding this comment.
No, that's still the case.
| // CHECK-LINEAR: OpCapability ComputeDerivativeGroupLinearKHR | ||
| // CHECK-LINEAR: OpExecutionMode %{{[^ ]*}} DerivativeGroupLinearKHR | ||
| // CHECK-QUADS: OpCapability ComputeDerivativeGroupQuadsKHR | ||
| // CHECK-QUADS: OpExecutionMode %{{[^ ]*}} DerivativeGroupQuadsKHR |
There was a problem hiding this comment.
Looks like we are making a guess based on the default values of the spec constants. That seems reasonable.
There was a problem hiding this comment.
Yes, I don't see a better way. I should probably add a warning that the determination may not be optimal if there are spec constants for NumThreads.
Fixes #3092