Implement the OpFRem instruction. #1249
Conversation
AlexDemydenko
force-pushed
the
GFXSW-22783
branch
from
2bfd22c to
cfcce2c
Compare
The test sute test_spirv_new from OpenCL-CTS fails. Algorithms of the fmod() function require synchronization between compilation procedure from *.cl and *.spv The clspv*.bc has an imlementation of the fmod() function. The source https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/generic/FMod.h The vulkan driver could have another version of OpFRem that is worst but good anouth to pass all graphics CTS tests.The vulkan driver may have another version of OpFRem which is worse but good enough to pass all CTS graphics tests. Change-Id: I016659a35c11365d969ebd3af23b1917f85b76c4
AlexDemydenko
force-pushed
the
GFXSW-22783
branch
from
cfcce2c to
c0bc112
Compare
| @@ -3449,7 +3455,6 @@ spv::Op SPIRVProducerPassImpl::GetSPIRVBinaryOpcode(Instruction &I) { | |||
| {Instruction::SDiv, spv::OpSDiv}, | |||
| {Instruction::FDiv, spv::OpFDiv}, | |||
| {Instruction::URem, spv::OpUMod}, | |||
| {Instruction::FRem, spv::OpFRem}, | |||
There was a problem hiding this comment.
Just removing the mapping to OpFRem feels wrong.
Maybe have a look at what we did for the division: #891
There was a problem hiding this comment.
Yes, I also don't like the idea to apply changes in spirv on the last pass instead of produce LLVM-IR on one of the early passes.
And I also started with ReplaceOpenCLBuiltinPass. The problem is that I'm dealing with an "Instruction", not a "Builtins" function.
The clvk drivers use the standard SPIRV-LLVM-Translator project. And this project converts spv::OpFMod and spv::OpFRem to Instruction::FRem (not as a function and not as a Builtins).
I need a pass that is very similar to ReplaceOpenCLBuiltinPass but deals with all the Instructions and can replace any of them not only "Functions".
I can create a new pass that can be placed after ReplaceOpenCLBuiltinPass. And add a flag something like "External spirv/IR" to enable this flag/pass only for IR generated by any external parser.
There was a problem hiding this comment.
Have any comments or ideas?
There was a problem hiding this comment.
In what range is the current operator failing?
Implementing the floating point reminder with integers feels like a big drop in performance.
Would x - y * trunc( x/y ) work, using the compliant fp division?
There was a problem hiding this comment.
- The problems accur all over the range, but always the delta in last few bits.
Example:
fmod(0.89177298546, 0.05671297759)
*.cl result = 0.04107832164
*.spv result = 0.04107832909
delta = 0.00000000745
-
"Floating point reminder" already exists in the compilation flow, in clspv*.bc. The problem is that it only works for *.cl -> compute.spv and doesn't work for kernel.spv -> comute.spv.
-
The algorithm that I use in the commit is based on what I get from cl -> * cpv flow.
-
Where is the problem. The OCL-CTS test has a list of OCL-functions that have equivalents in SpirV, and the test expects that simple kernels with these functions, one by one, from the spirv and cl sources must produce identical results.
There was a problem hiding this comment.
Are you planing to handle the cl_khr_il_program extension + SPIRV in future?
In the near future, I would answer no for my side (chromeos).
Is the InputLanguage option public with full support for any valid IL or is it just an option for debugging purposes that supports IL from a specific version of the LLVM FrontEnd?
The InputLanguage main goal is to be able to link OpenCL-C program together then it can be used for other things.
There was a problem hiding this comment.
I had discussion with other teams. I can assume that there are no requirements for this future now. But everyone mentions that it is a "Nice to have", and we need to try ta save it.
There some different options such as cpv-fork, cvk-fork, etc.
But the most preferable option, in our opinion, it is to inject such changes directly to you repository :).
My proposal.
I can create a new pass, something like LLVMTranslatorPatch, that should cover all the side effect that occur if the IL comes from SPIRV-LLVM-Translator.
To enable this pass, I will add llvm::cl::opt supportLLVMTranslator = false;
And I can add braces #ifdef SUPPORT_LLVM_TRANSLATOR with cmake-option if needed.
If it works for you, I'd be happy to prepare these changes for review.
There was a problem hiding this comment.
To move forward I need your response.
If it works for you I will be happy to prepare a new commit, if not then I will remove the pull request.
There was a problem hiding this comment.
@alan-baker what do you think?
There was a problem hiding this comment.
I'm not a big fan of build time controls. I wouldn't want to have another set of bots for this, but then we wouldn't be testing it regularly. More palatable to me would be a runtime option which enables/disables a pass. Could this be done as conditional transform in ReplaceLLVMIntrinsics?
|
The problem would be discussed in the #1325 |
The test sute test_spirv_new from OpenCL-CTS fails.
Algorithms of the fmod() function require synchronization
between compilation procedure from *.cl and *.spv
The clspv*.bc has an imlementation of the fmod() function.
The source https://github.com/llvm/llvm-project/blob/main/libc/src/__support/FPUtil/generic/FMod.h
The vulkan driver may have another version of OpFRem that is worse but good enough to pass all graphics CTS tests.