Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SPV_KHR_untyped_pointers - implement OpUntypedPrefetchKHR #2752

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

SPV_KHR_untyped_pointers - implement OpUntypedPrefetchKHR #2752

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fdadd5
SPV_KHR_untyped_pointers - implement OpUntypedPrefetchKHR
vmaksimo Oct 11, 2024
df7eabb
update headers tag
vmaksimo Oct 11, 2024
e5dd732
Merge remote-tracking branch 'origin/main' into untyped-prefetch
vmaksimo Oct 15, 2024
707d9b4
rewrite validation checks
vmaksimo Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions lib/SPIRV/SPIRVReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,6 +2475,36 @@ Value *SPIRVToLLVM::transValueWithoutDecoration(SPIRVValue *BV, Function *F,
BV, Builder.CreateIntrinsic(Intrinsic::expect, RetTy, {Val, ExpVal}));
}

case OpUntypedPrefetchKHR: {
// Do the same as transOCLBuiltinFromExtInst() but for OpUntypedPrefetchKHR.
auto *BC = static_cast<SPIRVUntypedPrefetchKHR *>(BV);

std::vector<Type *> ArgTypes =
transTypeVector(BC->getValueTypes(BC->getArguments()), true);
Type *RetTy = Type::getVoidTy(*Context);

std::string MangledName =
getSPIRVFriendlyIRFunctionName(OpenCLLIB::Prefetch, ArgTypes, RetTy);
Copy link
Contributor

@MrSidims MrSidims Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. if RW, locality and cache type optional parameters are set, then map the instruction to llvm.prefetch;
  2. 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");
  3. 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. (as before) if no optional parameters are set - map it to OpenCL prefetch call;
  2. 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);
  3. if optional parameter present and number of bytes != 64 - leave it as SPIR-V friendly call.

opaquifyTypedPointers(ArgTypes);

FunctionType *FT = FunctionType::get(RetTy, ArgTypes, false);
Function *F = M->getFunction(MangledName);
if (!F) {
F = Function::Create(FT, GlobalValue::ExternalLinkage, MangledName, M);
F->setCallingConv(CallingConv::SPIR_FUNC);
if (isFuncNoUnwind())
F->addFnAttr(Attribute::NoUnwind);
if (isFuncReadNone(OCLExtOpMap::map(OpenCLLIB::Prefetch)))
F->setDoesNotAccessMemory();
}

auto Args = transValue(BC->getValues(BC->getArguments()), F, BB);
CallInst *CI = CallInst::Create(F, Args, BC->getName(), BB);
setCallingConv(CI);
addFnAttr(CI, Attribute::NoUnwind);

return mapValue(BV, CI);
}
case OpExtInst: {
auto *ExtInst = static_cast<SPIRVExtInst *>(BV);
switch (ExtInst->getExtSetKind()) {
Expand Down
6 changes: 6 additions & 0 deletions lib/SPIRV/SPIRVWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add translation from prefetch llvm intrinsic.

if (BM->isAllowedToUseExtension(ExtensionID::SPV_KHR_untyped_pointers)) {
return BM->addUntypedPrefetchKHRInst(
transScavengedType(CI),
BM->getIds(transValue(getArguments(CI), BB)), BB);
}
}

return addDecorations(
Expand Down
77 changes: 77 additions & 0 deletions lib/SPIRV/libSPIRV/SPIRVInstruction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4225,5 +4225,82 @@ _SPIRV_OP(ConvertHandleToSamplerINTEL)
_SPIRV_OP(ConvertHandleToSampledImageINTEL)
#undef _SPIRV_OP

class SPIRVUntypedPrefetchKHR : public SPIRVInstruction {
public:
static const Op OC = OpUntypedPrefetchKHR;
static const SPIRVWord FixedWordCount = 3;

SPIRVUntypedPrefetchKHR(SPIRVType *Ty, std::vector<SPIRVWord> &TheArgs,
SPIRVBasicBlock *BB)
: SPIRVInstruction(FixedWordCount, OC, BB) {
setHasNoId();
setHasNoType();
PtrTy = TheArgs[0];
NumBytes = TheArgs[1];
if (TheArgs.size() > 2)
RW.push_back(TheArgs[2]);
if (TheArgs.size() > 3)
Locality.push_back(TheArgs[3]);
if (TheArgs.size() > 4)
CacheTy.push_back(TheArgs[4]);
assert(BB && "Invalid BB");
validate();
}

SPIRVUntypedPrefetchKHR() : SPIRVInstruction(OC) {
setHasNoId();
setHasNoType();
}

void validate() const override {
SPIRVInstruction::validate();
assert(getValueType(PtrTy)->isTypePointer());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use

 SPIRVErrorLog &SPVErrLog = this->getModule()->getErrorLog();
 SPVErrLog.checkError(....

assert(getValueType(PtrTy)->getPointerStorageClass() ==
StorageClassCrossWorkgroup);
assert(getValueType(NumBytes)->isTypeInt());
assert(RW.empty() || (RW.size() == 1 && getValueType(RW[0])->isTypeInt()));
assert(Locality.empty() ||
(Locality.size() == 1 && getValueType(Locality[0])->isTypeInt()));
assert(CacheTy.empty() ||
(CacheTy.size() == 1 && getValueType(CacheTy[0])->isTypeInt()));
}

void setWordCount(SPIRVWord TheWordCount) override {
SPIRVEntry::setWordCount(TheWordCount);
if (TheWordCount > 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, but why do we need these checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RW.resize(1);
if (TheWordCount > 4)
Locality.resize(1);
if (TheWordCount > 5)
CacheTy.resize(1);
}
const std::vector<SPIRVWord> getArguments() const {
std::vector<SPIRVWord> Args;
Args.push_back(PtrTy);
Args.push_back(NumBytes);
if (!RW.empty())
Args.push_back(RW[0]);
if (!Locality.empty())
Args.push_back(Locality[0]);
if (!CacheTy.empty())
Args.push_back(CacheTy[0]);
return Args;
}

SPIRVCapVec getRequiredCapability() const override {
return getVec(CapabilityUntypedPointersKHR);
}
std::optional<ExtensionID> getRequiredExtension() const override {
return ExtensionID::SPV_KHR_untyped_pointers;
}
_SPIRV_DEF_ENCDEC5(PtrTy, NumBytes, RW, Locality, CacheTy)
protected:
SPIRVId PtrTy;
SPIRVId NumBytes;
std::vector<SPIRVId> RW;
std::vector<SPIRVId> Locality;
std::vector<SPIRVId> CacheTy;
};

} // namespace SPIRV
#endif // SPIRV_LIBSPIRV_SPIRVINSTRUCTION_H
8 changes: 8 additions & 0 deletions lib/SPIRV/libSPIRV/SPIRVModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,9 @@ class SPIRVModuleImpl : public SPIRVModule {
SPIRVInstruction *addExpectKHRInst(SPIRVType *ResultTy, SPIRVValue *Value,
SPIRVValue *ExpectedValue,
SPIRVBasicBlock *BB) override;
SPIRVInstruction *addUntypedPrefetchKHRInst(SPIRVType *Ty,
std::vector<SPIRVWord> Args,
SPIRVBasicBlock *BB) override;

virtual SPIRVId getExtInstSetId(SPIRVExtInstSetKind Kind) const override;

Expand Down Expand Up @@ -1832,6 +1835,11 @@ SPIRVInstruction *SPIRVModuleImpl::addExpectKHRInst(SPIRVType *ResultTy,
BB);
}

SPIRVInstruction *SPIRVModuleImpl::addUntypedPrefetchKHRInst(
SPIRVType *Ty, std::vector<SPIRVWord> Args, SPIRVBasicBlock *BB) {
return addInstruction(new SPIRVUntypedPrefetchKHR(Ty, Args, BB), BB);
}

// Create AliasDomainDeclINTEL/AliasScopeDeclINTEL/AliasScopeListDeclINTEL
// instructions
template <typename AliasingInstType>
Expand Down
3 changes: 3 additions & 0 deletions lib/SPIRV/libSPIRV/SPIRVModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,9 @@ class SPIRVModule {
SPIRVValue *Value,
SPIRVValue *ExpectedValue,
SPIRVBasicBlock *BB) = 0;
virtual SPIRVInstruction *
addUntypedPrefetchKHRInst(SPIRVType *Ty, std::vector<SPIRVWord> Args,
SPIRVBasicBlock *BB) = 0;

virtual SPIRVId getExtInstSetId(SPIRVExtInstSetKind Kind) const = 0;

Expand Down
1 change: 1 addition & 0 deletions lib/SPIRV/libSPIRV/SPIRVOpCodeEnum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ _SPIRV_OP(UntypedAccessChainKHR, 4419)
_SPIRV_OP(UntypedInBoundsAccessChainKHR, 4420)
_SPIRV_OP(UntypedPtrAccessChainKHR, 4423)
_SPIRV_OP(UntypedInBoundsPtrAccessChainKHR, 4424)
_SPIRV_OP(UntypedPrefetchKHR, 4426)
_SPIRV_OP(GroupNonUniformRotateKHR, 4431)
_SPIRV_OP(SDotKHR, 4450)
_SPIRV_OP(UDotKHR, 4451)
Expand Down
2 changes: 1 addition & 1 deletion spirv-headers-tag.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
db5a00f8cebe81146cafabf89019674a3c4bf03d
efb6b4099ddb8fa60f62956dee592c4b94ec6a49
18 changes: 14 additions & 4 deletions test/extensions/INTEL/SPV_INTEL_cache_controls/decorate-prefetch-w-cache-controls.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
; RUN: llvm-as %s -o %t.bc
; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls -spirv-text %t.bc -o - | FileCheck %s --check-prefix=CHECK-SPIRV
; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls -spirv-text %t.bc -o - | FileCheck %s --check-prefixes=CHECK-SPIRV,CHECK-SPIRV-TYPED-PTRS

; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls %t.bc -o %t.spv
; RUN: spirv-val %t.spv
; RUN: llvm-spirv -r %t.spv --spirv-target-env=SPV-IR -o - | llvm-dis -o - | FileCheck %s --check-prefix=CHECK-LLVM

; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls,+SPV_KHR_untyped_pointers -spirv-text %t.bc -o - | FileCheck %s --check-prefixes=CHECK-SPIRV,CHECK-SPIRV-UNTYPED-PTRS

; RUN: llvm-spirv --spirv-ext=+SPV_INTEL_cache_controls,+SPV_KHR_untyped_pointers %t.bc -o %t.spv
; RUN: spirv-val %t.spv
; RUN: llvm-spirv -r %t.spv --spirv-target-env=SPV-IR -o - | llvm-dis -o - | FileCheck %s --check-prefix=CHECK-LLVM

target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-n8:16:32:64"
Expand All @@ -24,9 +31,12 @@ $_ZTSZ4mainEUlvE_ = comdat any
; CHECK-SPIRV: Decorate [[PTR_ID2:.*]] CacheControlLoadINTEL 1 1
; CHECK-SPIRV: Decorate [[PTR_ID3:.*]] CacheControlLoadINTEL 2 3

; CHECK-SPIRV: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID1]] [[#]]
; CHECK-SPIRV: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID2]] [[#]]
; CHECK-SPIRV: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID3]] [[#]]
; CHECK-SPIRV-TYPED-PTRS: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID1]] [[#]]
; CHECK-SPIRV-TYPED-PTRS: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID2]] [[#]]
; CHECK-SPIRV-TYPED-PTRS: ExtInst [[#]] [[#]] [[#]] prefetch [[PTR_ID3]] [[#]]
; CHECK-SPIRV-UNTYPED-PTRS: UntypedPrefetchKHR [[PTR_ID1]] [[#]]
; CHECK-SPIRV-UNTYPED-PTRS: UntypedPrefetchKHR [[PTR_ID2]] [[#]]
; CHECK-SPIRV-UNTYPED-PTRS: UntypedPrefetchKHR [[PTR_ID3]] [[#]]

; Check that the appropriate !spirv.Decorations are preserved after reverse
; translation
Expand Down
Loading