Report error when finding variadic function other than printf #2703
Conversation
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
lib/SPIRV/SPIRVWriter.cpp
Outdated
| @@ -814,6 +814,11 @@ SPIRVType *LLVMToSPIRVBase::transSPIRVOpaqueType(StringRef STName, | |||
| SPIRVType *LLVMToSPIRVBase::transScavengedType(Value *V) { | |||
| if (auto *F = dyn_cast<Function>(V)) { | |||
| FunctionType *FnTy = Scavenger->getFunctionType(F); | |||
| // VarArg functions other than printf are not supported in SPIR-V. | |||
| BM->getErrorLog().checkError( | |||
| !(FnTy->isVarArg() && F->getName() != "printf"), | |||
There was a problem hiding this comment.
Is this test too strict? In the test array_of_matrices.ll, I see:
declare dso_local spir_func i32 @_Z18__spirv_ocl_printfPU3AS2Kcz(ptr addrspace(2), ...)
There was a problem hiding this comment.
I agree, there is mangling, and at least 3 names for the printf, so F->getName() != "printf" would not work properly.
There was a problem hiding this comment.
I considered F->getName().contains("printf"), but thought that was too loose. What do you think, would that be better?
There was a problem hiding this comment.
In SPIR-V Backend we have something along the lines of
if ((DemangledName.starts_with("__spirv_ocl_printf(") ||
DemangledName.starts_with("printf(")) ...
so I guess as a first approach this could work after you've got DemangledName from F->getName() from llvm::itaniumDemangle.
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
| ; RUN: not llvm-spirv %t.bc 2>&1 | FileCheck %s | ||
|
|
||
| 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-G1" | ||
| target triple = "spir64_unknown_unknown" |
There was a problem hiding this comment.
spir64-unknown-unknown
|
|
||
| define i32 @foo() nounwind { | ||
| call spir_func void (i32, i32, ...) @for__issue_diagnostic(i32 noundef 41, i32 noundef 0) | ||
| ret i32 0 |
There was a problem hiding this comment.
two spaces before ret :)
| DemangledName.starts_with("__spirv_ocl_printf(")), | ||
| SPIRVEC_UnsupportedVarArgFunction); | ||
| } | ||
|
|
Signed-off-by: Marcos Maronas <[email protected]>
lib/SPIRV/SPIRVWriter.cpp
Outdated
| (DemangledName.starts_with("printf(") || | ||
| DemangledName.starts_with("__spirv_ocl_printf(")), |
There was a problem hiding this comment.
oclIsBuiltin demangles "printf" to "__spirv_ocl_printf". I think only checking for: DemangledName.starts_with("__spirv_ocl_printf") should be sufficient. I think we should possibly be able to check for equality instead of starts_with.
Also does DemangledName include an open parenthesis?
oclIsBuiltin starts with:
SPIRVUtil.cpp:bool oclIsBuiltin(StringRef Name, StringRef &DemangledName, bool IsCpp) {
SPIRVUtil.cpp- if (Name == "printf") {
SPIRVUtil.cpp- DemangledName = "__spirv_ocl_printf";
SPIRVUtil.cpp- return true;
SPIRVUtil.cpp- }
There was a problem hiding this comment.
I have actually realized none of printf variants can reach this piece of code because they are skipped in
SPIRV-LLVM-Translator/lib/SPIRV/SPIRVWriter.cpp
Line 5789 in 7dacb7c
isBuiltinTransToExtInst(&F) returns true for printf, so it is just skipped and never added to neither Decls nor Defs. Considering this, I think we should be safe by just checking if the function is variadic to report the error at this point. What do you think? Also tagging the rest of approvers to gather their opinions: @svenvh @MrSidims @VyacheslavLevytskyy
Signed-off-by: Marcos Maronas <[email protected]>
maarquitos14
requested review from
svenvh,
VyacheslavLevytskyy and
MrSidims
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
Signed-off-by: Marcos Maronas <[email protected]>
Variadic functions are not supported in SPIR-V, the only known exception is printf.