Enable translation of ac_fixed functions #614
Conversation
This patch introduces SPV_INTEL_arbitrary_precision_fixed_point extension, which adds operations for arbitrary precision fixed point numbers. The ac_fixed datatype is represented as a pseudo type using OpTypeInt. Its parameters are: * W - total width of the datatype. It's encoded in the width of the OpTypeInt. * I - determines the position of the decimal point. * S - determines if this is a signed or an unsigned number. Full specification (intel/llvm#1934): https://github.com/intel/llvm/blob/2f6e965e686354fbb25f9c177a667a646de302eb/sycl/doc/extensions/SPIRV/SPV_INTEL_arbitrary_precision_fixed_point.asciidoc
lib/SPIRV/SPIRVReader.cpp
Outdated
| FunctionType *FT = FunctionType::get(RetTy, ArgTys, false); | ||
|
|
||
| Op OpCode = Inst->getOpCode(); | ||
| Function *Func = Function::Create(FT, GlobalValue::ExternalLinkage, |
There was a problem hiding this comment.
Do you have a test where the same fixed point funciton/opcode is encountered twice? I would expect that Funciton::Create will declare the same buit-in each time it is called and it will add .1, .2, etc. suffixes to declaration name. You probably interested in llvm::Module::getOrInsertFunction`
There was a problem hiding this comment.
Oh, that's a good point, thanks! I will fix that and add the appropriate test case.
There was a problem hiding this comment.
This getOrInsertFunction is not perfect, though. If the same function is called twice - it works fine, but if the same function has different signatures and return types - a huge bitcast is being created:
call i13 bitcast (i5 (i13, i1, i32, i32, i32, i32)* @intel_arbitrary_fixed_sqrt to i13 (i5, i1, i32, i32, i32, i32)*)(i5 %5, i1 false, i32 2, i32 2, i32 0, i32 0)
Not sure what is better - bitcast or .N at the end of the function name. What do you think of this?
There was a problem hiding this comment.
It depends on what can be handled by the consumer of that LLVM IR which is translated from the SPIR-V.
I guess that both variants are not really acceptable. Usually, such overloads are distinguished by mangling and .1, .2 is some kind of it, also it is common that LLVM IR representation after the translation is similar to/the same as LLVM IR representation before translation - probably this is a way you need to go?
|
|
||
| FunctionCallee FCallee = M->getOrInsertFunction(FuncName, FT); | ||
|
|
||
| if (Function *Fn = dyn_cast<Function>(FCallee.getCallee())) { |
There was a problem hiding this comment.
Should we check if this is Function*? What if this is not so?
There was a problem hiding this comment.
We check that it is a Function* with dyn_cast<Function>. Probably we might want to check that FCallee.getCallee() doesn't return nullptr. We should use dyn_cast_or_null in this case.
There was a problem hiding this comment.
Well, actually we expect that it will be always Function* because we insert function call on the line above.
I would rather use just cast<Function> call without an if check.
| !1 = !{i32 1, i32 2} | ||
| !2 = !{i32 4, i32 100000} | ||
| !3 = !{} | ||
| !4 = !{!"Intel(R) oneAPI DPC++ Compiler 2021.1 (YYYY.x.0.MMDD)"} |
There was a problem hiding this comment.
I suppose that this test should be recompiled by opensource version of clang.
| CallInst *SPIRVToLLVM::transFixedPointInst(SPIRVInstruction *BI, | ||
| BasicBlock *BB) { | ||
| // LLVM fixed point functions return value: | ||
| // iN |
|
Updated review has been created in PR 653. |
Should we close this one and follow up in #653 ? |
Yes, it would be good to close this one as outdated. |
This patch introduces SPV_INTEL_arbitrary_precision_fixed_point extension,
which adds operations for arbitrary precision fixed point numbers.
The ac_fixed datatype is represented as a pseudo type using OpTypeInt. Its
parameters are:
Full specification (intel/llvm#1934):
https://github.com/intel/llvm/blob/2f6e965e686354fbb25f9c177a667a646de302eb/sycl/doc/extensions/SPIRV/SPV_INTEL_arbitrary_precision_fixed_point.asciidoc