Repository navigation
[SYCL][libdevice] improve complex trig and pi ulp - #23375
Conversation
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 1
Open (7)
id<1>does not support arithmetic/indexing the way this kernel uses it (4 * iandd_in[i]).… · New Same issue as incomplex_math.hpp:__has_include(<version>)should be guarded (e.g.,… · New__has_includeis not guaranteed to be available on all preprocessors, and using it unguarded can… · New These explicit specializations define non-inlinevariables in a header. If this header is… · New These explicit specializations define non-inlinevariables in a header. If this header is… · New These explicit specializations define non-inlinevariables in a header. If this header is… · New These explicit specializations define non-inlinevariables in a header. If this header is… · New
What changed in this PR
This PR improves correctness of complex inverse trig/hyperbolic functions for large-magnitude inputs on device by avoiding catastrophic cancellation and using a more reliable value of π, and adds/extends end-to-end coverage for the affected cases.
Changes:
- Add a new DeviceLib E2E test to compare
std::acos/asin/acosh/asinhon device vs host for large arguments. - Update complex math implementations (
acos/acosh/asinhand π constant selection) to avoid cancellation for negative real parts and fix π rounding issues. - Extend existing complex math test cases with additional “large argument” inputs for inverse functions.
| File | Description |
|---|---|
| sycl/test-e2e/DeviceLib/std_complex_inverse_trig_large_args.cpp | Adds a new E2E test exercising large-argument inverse complex functions on device. |
| sycl/test-e2e/Complex/sycl_complex_math_test_cases.hpp | Refactors default test values and appends large-argument cases for inverse functions. |
| sycl/include/sycl/ext/oneapi/experimental/complex/detail/complex_math.hpp | Switches π computation to a constant and updates inverse functions to avoid cancellation on negative real inputs. |
| libdevice/fallback-complex.hpp | Mirrors cancellation-avoidance logic and π constant usage for float complex fallbacks. |
| libdevice/fallback-complex-fp64.hpp | Mirrors cancellation-avoidance logic and π constant usage for double complex fallbacks. |
| libdevice/device_complex.h | Introduces a shared π definition used by libdevice complex implementations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
|
The failure in llvm/sycl/test-e2e/USM/memops2d/memcpy2d_device_to_host.cpp (or device_to_device.cpp) on Arc is being seen by other PR as well. |
|
@cperkinsintel could you please add libdevice owner to review this first? |
|
Hi, @cperkinsintel |
|
@jinge90 - yeah, I think something like this could be applied to libcxx upstream. I've never opened a PR against that, have you? @KseniyaTikhomirova - can you review as the representative from llvm-reviewers-runtime ? |
Hi, @cperkinsintel |
KseniyaTikhomirova
left a comment
There was a problem hiding this comment.
It is hard for me to estimate correctness of applied math changes so I reviewed this from language perspective only.
Signed-off-by: Chris Perkins <chris.perkins@intel.com>
KseniyaTikhomirova
left a comment
There was a problem hiding this comment.
SYCL RT changes LGTM
|
@intel/llvm-gatekeepers please consider merging |


Some of the inverse trig functions (acos, asin, acosh asinh) over complex numbers in SYCL have two problems:
z + sqrt(z^2 -+ 1)cancels catastrophically, its truevalue is smaler than the rounding error of its two terms.