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

Fix OCL builtins translation in case SPV_KHR_untyped_pointers is enabled #2723

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

Fix OCL builtins translation in case SPV_KHR_untyped_pointers is enabled #2723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
173be58
Fix OCL builtins translation in case SPV_KHR_untyped_pointers is enabled
vmaksimo Sep 19, 2024
f5295ec
Disable validation
vmaksimo Sep 19, 2024
b8ba0c5
Address code review comments
vmaksimo Sep 20, 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
11 changes: 11 additions & 0 deletions lib/SPIRV/SPIRVReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4757,6 +4757,17 @@ Instruction *SPIRVToLLVM::transOCLBuiltinFromExtInst(SPIRVExtInst *BC,
"Not OpenCL extended instruction");

std::vector<Type *> ArgTypes = transTypeVector(BC->getArgTypes(), true);
for (unsigned I = 0; I < ArgTypes.size(); I++) {
Copy link
Contributor

@LU-JOHN LU-JOHN Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Does function transBuiltinFromInst() also need this special handling? It also calls transTypeVector() and gets a mangled name. Consider putting this special processing within transTypeVector().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My experiments show that e.g. for OpEnqueKernel we don't meet untyped variables in arguments, but the mangling is still wrong - should be fixed elsewhere.
It's still possible to unify this fix inside transTypeVector(), but not sure if it'd be used by transBuiltinFromInst().

// Special handling for "truly" untyped pointers to preserve correct OCL
// bultin mangling.
if (isa<PointerType>(ArgTypes[I]) && BC->getArg(I)->isUntypedVariable()) {
auto *BVar = static_cast<SPIRVUntypedVariableKHR *>(BC->getArg(I));
ArgTypes[I] = TypedPointerType::get(
transType(BVar->getDataType()),
SPIRSPIRVAddrSpaceMap::rmap(BVar->getStorageClass()));
}
}

Type *RetTy = transType(BC->getType());
std::string MangledName =
getSPIRVFriendlyIRFunctionName(ExtOp, ArgTypes, RetTy);
Expand Down
1 change: 1 addition & 0 deletions lib/SPIRV/libSPIRV/SPIRVEntry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ class SPIRVEntry {
bool isVariable() const {
return OpCode == OpVariable || OpCode == OpUntypedVariableKHR;
}
bool isUntypedVariable() const { return OpCode == OpUntypedVariableKHR; }
bool isEndOfBlock() const;
virtual bool isInst() const { return false; }
virtual bool isOperandLiteral(unsigned Index) const {
Expand Down
14 changes: 8 additions & 6 deletions lib/SPIRV/libSPIRV/SPIRVInstruction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1995,14 +1995,16 @@ class SPIRVExtInst : public SPIRVFunctionCallGeneric<OpExtInst, 5> {
}
std::vector<SPIRVValue *> getArgValues() {
std::vector<SPIRVValue *> VArgs;
for (size_t I = 0; I < Args.size(); ++I) {
if (isOperandLiteral(I))
VArgs.push_back(Module->getLiteralAsConstant(Args[I]));
else
VArgs.push_back(getValue(Args[I]));
}
for (size_t I = 0; I < Args.size(); ++I)
VArgs.push_back(getArgValue(I));
return VArgs;
}
SPIRVValue *getArgValue(SPIRVWord I) {
if (isOperandLiteral(I))
return Module->getLiteralAsConstant(Args[I]);
return getValue(Args[I]);
}

std::vector<SPIRVType *> getArgTypes() {
std::vector<SPIRVType *> ArgTypes;
auto VArgs = getArgValues();
Expand Down
31 changes: 21 additions & 10 deletions test/transcoding/float16.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,34 @@
; RUN: llvm-as %s -o %t.bc
; RUN: llvm-spirv %t.bc -o %t.spv
; RUN: spirv-val %t.spv
; RUN: llvm-spirv %t.spv -to-text -o - | FileCheck %s --check-prefix=CHECK-SPIRV
; RUN: llvm-spirv %t.spv -to-text -o - | FileCheck %s --check-prefixes=CHECK-SPIRV,CHECK-SPIRV-NOEXT
; RUN: llvm-spirv -r %t.spv -o %t.rev.bc
; RUN: llvm-dis %t.rev.bc -o - | FileCheck %s --check-prefix=CHECK-LLVM

; Verify that even though we use the fract instruction with untyped pointers enabled,
; the SPV binary is valid and we get exactly the same output IR after the reverse translation.
; RUN: llvm-spirv %t.bc -o %t.spv --spirv-ext=+SPV_KHR_untyped_pointers
Copy link
Contributor

@LU-JOHN LU-JOHN Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How did you pick this file to do additional testing with SPV_KHR_untyped_pointers?
Please add a comment explaining why this additional testing was added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I'm just running all the tests with the extension enabled and trying to understand where the real fixes should be made, or where we just need to update Checks.
This one is testing OCL builtin so I've checked if we are working fine with them when the extension enabled (turned out that not).
Will add the comment about additional testing though.

; TODO: enable back once spirv-tools are updated
; R/UN: spirv-val %t.spv
; RUN: llvm-spirv %t.spv -to-text -o - | FileCheck %s --check-prefixes=CHECK-SPIRV,CHECK-SPIRV-EXT
; RUN: llvm-spirv -r %t.spv -o %t.rev.bc
; RUN: llvm-dis %t.rev.bc -o - | FileCheck %s --check-prefix=CHECK-LLVM

source_filename = "math_builtin_float_half.cpp"
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"
target triple = "spirv64-unknown-unknown"

; CHECK-SPIRV: 3 TypeFloat [[HALF:[0-9]+]] 16
; CHECK-SPIRV: 4 TypePointer [[HALFPTR:[0-9]+]] 7 [[HALF]]
; CHECK-SPIRV: 4 TypeVector [[HALFV2:[0-9]+]] [[HALF]] 2
; CHECK-SPIRV: 4 TypePointer [[HALFV2PTR:[0-9]+]] 7 [[HALFV2]]
; CHECK-SPIRV: 4 Constant [[HALF]] [[CONST:[0-9]+]] 14788
; CHECK-SPIRV: 4 Variable [[HALFPTR]] [[ADDR:[0-9]+]] 7
; CHECK-SPIRV: 4 Variable [[HALFV2PTR]] [[ADDR2:[0-9]+]] 7
; CHECK-SPIRV: 7 ExtInst [[HALF]] [[#]] 1 fract [[CONST]] [[ADDR]]
; CHECK-SPIRV: 7 ExtInst [[HALFV2]] [[#]] 1 fract [[#]] [[ADDR2]]
; CHECK-SPIRV: TypeFloat [[HALF:[0-9]+]] 16
; CHECK-SPIRV-NOEXT: TypePointer [[HALFPTR:[0-9]+]] 7 [[HALF]]
; CHECK-SPIRV-EXT: TypeUntypedPointerKHR [[HALFPTR:[0-9]+]] 7
; CHECK-SPIRV: TypeVector [[HALFV2:[0-9]+]] [[HALF]] 2
; CHECK-SPIRV: TypePointer [[HALFV2PTR:[0-9]+]] 7 [[HALFV2]]
; CHECK-SPIRV: Constant [[HALF]] [[CONST:[0-9]+]] 14788
; CHECK-SPIRV-NOEXT: Variable [[HALFPTR]] [[ADDR:[0-9]+]] 7
; CHECK-SPIRV-EXT: UntypedVariableKHR [[HALFPTR]] [[ADDR:[0-9]+]] 7 [[HALF]]
; CHECK-SPIRV: Variable [[HALFV2PTR]] [[ADDR2:[0-9]+]] 7
; CHECK-SPIRV: ExtInst [[HALF]] [[#]] 1 fract [[CONST]] [[ADDR]]
; CHECK-SPIRV: ExtInst [[HALFV2]] [[#]] 1 fract [[#]] [[ADDR2]]

; CHECK-LLVM: %addr = alloca half
; CHECK-LLVM: %addr2 = alloca <2 x half>
Expand Down
Loading