Conversation
alan-baker
left a comment
alan-baker
left a comment
There was a problem hiding this comment.
What about places where we insert spirv ops earlier in the flow? I feel like there may be cases of missed operands.
lib/Compiler.cpp
Outdated
| if (clspv::Option::VulkanMemoryModel() && | ||
| clspv::Option::SpvVersion() < clspv::Option::SPIRVVersion::SPIRV_1_5) { | ||
| llvm::errs() << "vulkan memory model is not supported for spirv versions " | ||
| "less that 1.5\n"; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
The OpExtension is required prior to 1.5, but the functionality is available so this error can be dropped.
lib/SPIRVProducerPass.cpp
Outdated
| ; | ||
|
|
||
| if (clspv::Option::VulkanMemoryModel()) { | ||
| addSPIRVInst<kExtensions>(spv::OpExtension, "SPV_KHR_vulkan_memory_model"); |
There was a problem hiding this comment.
The extension is only required if the version is < 1.5
lib/SPIRVProducerPass.cpp
Outdated
| return uniformPointer; | ||
| } | ||
|
|
||
| bool SPIRVProducerPassImpl::isCoherentResource(Value *Ptr) { |
There was a problem hiding this comment.
This seems like it will fail if the address calculation includes a phi, select, or function parameter. Previously those cases would all have been covered by the coherent decoration.
lib/SPIRVProducerPass.cpp
Outdated
| addSPIRVInst<kCapabilities>(spv::OpCapability, | ||
| spv::CapabilityVulkanMemoryModelDeviceScopeKHR); |
There was a problem hiding this comment.
It's not clear to me that we'd want device scope by default. I think QueueFamily is probably more appropriate. I'd suggest you take a look at clvk, but I expect it doesn't use different families.
This could be an extra command line option. Probably you'd want to introduce a helper function to get the right scope.
| // CHECK-NOT: OpDecorate {{.*}} Coherent | ||
| // CHECK: [[uint:%[_a-zA-Z0-9]+]] = OpTypeInt 32 0 | ||
| // CHECK: [[DEVICE_SCOPE:%[_a-zA-Z0-9]+]] = OpConstant [[uint]] 1 | ||
| // CHECK: {{.*}} = OpLoad [[uint]] {{.*}} MakePointerVisible|NonPrivatePointer [[DEVICE_SCOPE]] |
There was a problem hiding this comment.
What about the store in bar. If x is selected it should be coherent.
lib/SPIRVProducerPass.cpp
Outdated
| const auto coherent = | ||
| unsigned(dyn_cast<ConstantInt>( | ||
| Call->getArgOperand(ClspvOperand::kResourceCoherent)) | ||
| ->getZExtValue()); | ||
|
|
||
| if (clspv::Option::VulkanMemoryModel() && coherent == 1) { | ||
| Ops << (spv::MemoryAccessNonPrivatePointerMask | | ||
| spv::MemoryAccessMakePointerVisibleMask) | ||
| << getSPIRVInt32Constant(spv::ScopeDevice); | ||
| } |
There was a problem hiding this comment.
I think this is always unnecessary. These loads occur for samplers and images. Those handle loads come from UniformConstant and the handles themselves are never modified.
|
|
||
| bool isCoherent = isCoherentResource(Call->getArgOperand(0)); | ||
|
|
||
| if (clspv::Option::VulkanMemoryModel() && isCoherent) { |
There was a problem hiding this comment.
I don't see any coverage of this in the tests. In fact, I'm not sure clspv can generate a coherent image right now. That is likely a bug since where this functionality wasn't updated for read_write images. It would need a change in descriptor allocation. Please either fix that or file an issue to address it.
Separately, this could be tested using IR only. Take a look at the tests in Spv1p4 for how to run SPIRVProducer only tests.
|
|
||
| bool isCoherent = isCoherentResource(Call->getArgOperand(0)); | ||
|
|
||
| if (clspv::Option::VulkanMemoryModel() && isCoherent) { |
There was a problem hiding this comment.
Same as above, please add unit test coverage.
omarahmed1111
force-pushed
the
Add-vulkan-memory-model
branch
from
364c459 to
055bbb0
Compare
|
Is there a plan to move forward with this PR? I am working on a tool to verify SPIRV wrt the vulkan memory model and having clspv generate such kind of code would be super useful. I wanted to test the code in this PR, but unfortunately it does not build for me. |
We'd like to eventually finish this, but I can't commit to a time frame when that might occur. In the meantime, you should be able to get equivalent functionality by the SPIRV-Tools optimizer using |
|
Thanks @alan-baker, this was super useful! |
5 participants
PR Changes
vulkan-memory-modeloption to clspvAvailableandVisibleon read and write instructionsRelated issue
This contribution is being made by Codeplay on behalf of Samsung.