[BugFix][Target][LLVM] Use libm for asin/acos instead of buggy inline Taylor#19567
Open
swjng wants to merge 2 commits into
Open
[BugFix][Target][LLVM] Use libm for asin/acos instead of buggy inline Taylor#19567swjng wants to merge 2 commits into
swjng wants to merge 2 commits into
Conversation
The `tirx.asin` LLVM legalization rule used a 6-term Taylor series for |x| < 0.5 with incorrect coefficients. The recurrence multipliers (`9/40`, `25/112`, `1225/3456`, `3969/28160`) do not match the standard asin Taylor series ratios (`9/20`, `25/42`, `49/72`, `81/110`), so the series under-counted higher-order terms and lost roughly 1e-3 of precision in the mid-range (x ≈ 0.3–0.5) — well over a thousand ULP at float32. `tirx.acos` inherits the same error via `π/2 - asin(x)` for |x| < 0.5. The Taylor branch was added as the original implementation in apache#17945 with no libm fallback; apache#18582 later patched the boundary at |x| ≥ 0.5 by switching to the libm extern. There is no evidence the inline series was ever a deliberate fast-path — it was simply incomplete. This change drops the inline series and routes the whole input range through the existing libm extern (`asinf`/`acosf`), keeping only the out-of-range NaN guard. ULP-grade precision is restored across the full domain. The Asin/Acos cases that were commented out of `test_unary` with a "Taylor approximation precision loss" TODO are re-enabled. Fixes apache#19563.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request replaces the Taylor series approximations for asin and acos with direct libm calls in the LLVM intrinsic rules to resolve precision issues, and enables the corresponding ONNX frontend tests. Review feedback indicates that the manual NaN guards for these operations are now redundant since the library functions handle out-of-range inputs natively, and suggests simplifying the return expressions to reduce complexity in the generated TIR.
Per review feedback: with the inline Taylor branch removed, the manual out-of-range Select(out_range, NaN, lib_result) is redundant. The libm externs (asinf / acosf) already return NaN for |x| > 1 by C11 and IEEE 754, so the guard only adds dead TIR. Verified against ±2.0 inputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tirx.asin's LLVM legalize used a 6-term Taylor series for|x| < 0.5with wrong recurrence coefficients. The ratios in the code (9/40,25/112,1225/3456,3969/28160) don't match the real asin series (9/20,25/42,49/72,81/110), so mid-range inputs lose ~1e-3 of precision — over 1000 float32 ULP.acosinherits it viaπ/2 − asin(x).The Taylor branch was added in #17945 as the initial implementation, with no libm fallback. #18582 later patched only
|x| ≥ 0.5by routing to the libm extern, leaving the buggy mid-range in place. I see no evidence the inline series was an intentional fast-path.Fix
Drop the inline series, route the whole domain through the existing
asinf/acosfextern, keep the out-of-range NaN guard. Max error overx ∈ [-1, 1]drops to 2.4e-7 (ULP-grade).Tests
Asin/Acosintest_unary(they were commented out with a TODO about Taylor precision loss).test_asin_acos_boundary_values(Fix ACOS precision issue for boundary values (x=±1.0) #18582) still passes.If the inline polynomial was intentional for some target/path, please flag it — I'll restore it with corrected coefficients instead.
Atanis still disabled; that's a separatex² + 1overflow bug (#19560).Fixes #19563.