SPV_KHR_untyped_pointers - implement OpUntypedPrefetchKHR #2752
Conversation
When the extension is enabled, we should replace `prefetch` OpenCL ExtInst with the `OpUntypedPrefetchKHR` instruction. Spec: https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_untyped_pointers.html
|
|
||
| void validate() const override { | ||
| SPIRVInstruction::validate(); | ||
| assert(getValueType(PtrTy)->isTypePointer()); |
There was a problem hiding this comment.
Lets use
SPIRVErrorLog &SPVErrLog = this->getModule()->getErrorLog();
SPVErrLog.checkError(....
|
|
||
| void setWordCount(SPIRVWord TheWordCount) override { | ||
| SPIRVEntry::setWordCount(TheWordCount); | ||
| if (TheWordCount > 3) |
There was a problem hiding this comment.
I might be missing something, but why do we need these checks?
There was a problem hiding this comment.
This is the way to handle optional arguments that is used through the translator's codebase. For the current test case without optional parameters these checks are not used, but need to verify if they are in case we have optional operands.
| Type *RetTy = Type::getVoidTy(*Context); | ||
|
|
||
| std::string MangledName = | ||
| getSPIRVFriendlyIRFunctionName(OpenCLLIB::Prefetch, ArgTypes, RetTy); |
There was a problem hiding this comment.
I started to remember (missed it in our chat). We need to map the instruction to llvm.prefetch intrinsic. So probably the logic should be:
- if RW, locality and cache type optional parameters are set, then map the instruction to llvm.prefetch;
- else if some of the optional parameters present, then we should emit SPIR-V friendly LLVM IR call (btw, @alan-baker I believe some routine lines should be added to the spec like: "if Cache Type present, then other optional parameters must be set");
- if no optional parameters are set - map it to OpenCL prefetch call.
Note, No.2 is discussible, we may want to still emit llvm intrinsic but with some default values set. In the case No.2 RW will always be set, which is good as we won't need to guess a default value, locality can be defaulted to '0'. Not sure though what to use for Cache Type, '1' seems reasonable to me. Would be nice to hear from @alan-baker @bashbaug and @svenvh about that.
There was a problem hiding this comment.
We could add that restriction in the future if that is the only practical use case. I left it vague because I am not familiar with generating the equivalent LLVM intrinsic.
As for defaults I simply tried to provide the same arguments as the LLVM intrinsics, though I can't imagine how instruction cache fetching could be done via OpenCL C. So for Cache Type I'd expect it's always safe to default to data.
There was a problem hiding this comment.
About p.1, I'm not sure we should use llvm.prefetch intrinsic for now because it does not contain the number of bytes to prefetch field.
The similar concern is for forward translation. Although llvm.prefetch contains RW, Locality and Cache Type fields, it does not have a number of bytes to prefetch, which is a mandatory field in OpUntypedPrefetchKHR instruction.
There was a problem hiding this comment.
it does not contain the number of bytes to prefetch field
Indeed. Probably it can go to another PR, but we can add the following logic:
- (as before) if no optional parameters are set - map it to OpenCL prefetch call;
- if one of the optional parameters (RW, locality or cache type) optional parameters is set and number of bytes = 64, then map it on llvm.prefetch (on x86-64 prefetch would prefetch 64 bytes, other magic number is also possible);
- if optional parameter present and number of bytes != 64 - leave it as SPIR-V friendly call.
| @@ -5329,6 +5329,12 @@ SPIRVValue *LLVMToSPIRVBase::transDirectCallInst(CallInst *CI, | |||
| BM->addExtension( | |||
| ExtensionID::SPV_EXT_relaxed_printf_string_address_space); | |||
| } | |||
| } else if (DemangledName.find("__spirv_ocl_prefetch") != StringRef::npos) { | |||
There was a problem hiding this comment.
Please also add translation from prefetch llvm intrinsic.
When the extension is enabled, we should replace
prefetchOpenCL ExtInst with theOpUntypedPrefetchKHRinstruction.Spec:
https://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_untyped_pointers.html