[NFCI][SYCL] Refactor selection of FP builtin calls #16966
Conversation
This is an upstream from a closed source repository. Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>
MrSidims
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
Can we please add some details about this change in the PR description? Thanks |
MrSidims
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
|
Can you please comment on why we do not have any tests added here? Thanks |
| }; | ||
|
|
||
| FPBuiltinReplacement(Kind K, const StringRef &ImplName = StringRef()) | ||
| : RepKind(K), AltMathFunctionImplName(ImplName) { |
There was a problem hiding this comment.
Do we want to assign to AltMathFunctionImplName even if K is not set to ReplaceWithAltMathFunction?
There was a problem hiding this comment.
I think not. In this case we should to either error out or attempt to lower to standard to LLVM instructions/intrinsics or target specific intrinsics if applicable (like nvvm approx math intrinsics).
|
Thanks @asudarsa , going one by one for your questions
No as details exposes internal code, that we don't plan to upstream.
Because it's a refactoring (aka NFC change). The original patch contained tests, but they are exposing other customizations that we don't plan to upstream. |
| return FPBuiltinReplacement(FPBuiltinReplacement::ReplaceWithLLVMIR); | ||
| return FPBuiltinReplacement(FPBuiltinReplacement::Unexpected0dot5); | ||
| } | ||
| // AltMathLibrary don't have implementation for CUDA approximate precision |
There was a problem hiding this comment.
Just curious: Does 'Accuracy > 0.5' map to 'approximate precision'?
Thanks
There was a problem hiding this comment.
'Accuracy > 0.5' maps on 'not-precise'. Regarding if it maps on nvvm approx intrinsics/nvptx approx instruction - it depends on the operation and what ptx spec mandates.
More details is in the discussion with Joshua in #16714 .
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
MrSidims
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
@intel/llvm-gatekeepers please help with the merge |
This is an upstream from a closed source repository.
Co-author: Stasenko, Alexander P alexander.p.stasenko@intel.com