Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

math/cmplx: Tan incorrect on s390x for large arguments #37854

Closed
bmkessler opened this issue Mar 14, 2020 · 5 comments
Closed

math/cmplx: Tan incorrect on s390x for large arguments #37854

bmkessler opened this issue Mar 14, 2020 · 5 comments
Assignees
@Vishwanatha-HD
Labels
arch-s390x Issues solely affecting the s390x architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bmkessler
Copy link
Contributor

s390x fails the tests for Tan with large arguments due to incorrect argument reduction in the trigonometric routines for the base math packages.

--- FAIL: TestTan (0.00s)
    cmath_test.go:839: Tan((-1.329227995784916e+36+0i)) = (NaN+NaNi), want (0.40806638884180424+0i)
    cmath_test.go:839: Tan((1.7668470647783843e+72+0i)) = (NaN+NaNi), want (-0.37603456702698074+0i)
    cmath_test.go:839: Tan((2.037035976334486e+90+0i)) = (NaN+NaNi), want (4.60901287677811+0i)
    cmath_test.go:839: Tan((-3.1217485503159922e+144+0i)) = (NaN+NaNi), want (3.391359650547799+0i)
    cmath_test.go:839: Tan((1.8919697882131776e+69+0i)) = (NaN+NaNi), want (-6.76813854009065+0i)
    cmath_test.go:839: Tan((-2.514859209672214e+105+0i)) = (NaN+NaNi), want (-0.7641769501660493+0i)

This is due to the fix for #31566, but the underlying issue has been present since #6794 was fixed. The temporary solution after that change was to just disable the tests for s390x.

I think the s390x assembly implementations need to be fixed or they should just revert to the pure go versions for large arguments.

@griesemer @mundaym
@gopherbot
Copy link

Change https://golang.org/cl/223457 mentions this issue: math/cmplx: disable TanHuge test on s390x

gopherbot pushed a commit that referenced this issue Mar 14, 2020
@bmkessler @robpike
math/cmplx: disable TanHuge test on s390x
d774d97 
s390x has inaccurate range reduction for the assembly routines
in math so these tests are diabled until these are corrected.

Updates #37854

Change-Id: I1e26acd6d09ae3e592a3dd90aec73a6844f5c6fe
Reviewed-on: https://go-review.googlesource.com/c/go/+/223457
Run-TryBot: Brian Kessler <brian.m.kessler@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@cagedmantis cagedmantis added this to the Backlog milestone Mar 16, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 16, 2020
@mundaym mundaym added the arch-s390x Issues solely affecting the s390x architecture. label Dec 2, 2020
@Vishwanatha-HD Vishwanatha-HD self-assigned this Feb 19, 2023
@Vishwanatha-HD Vishwanatha-HD added Testing An issue that has been verified to require only test changes, not just a test failure. compiler/runtime Issues related to the Go compiler and/or runtime. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 19, 2023
@Vishwanatha-HD
Copy link
Contributor

Found a fix and made code changes. Tested the fix and made sure that its working fine. I will raise my code changes for review in Gerrit...

@Vishwanatha-HD Vishwanatha-HD added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 19, 2023
@mknyszek
Copy link
Contributor

CC @golang/s390x

@Vishwanatha-HD
Copy link
Contributor

I have raised a CL to fix this issue and its under review. Please note that my fix has been tested and its working fine.

Please find the link below:
https://go-review.googlesource.com/c/go/+/470595

Benchmark details of Tan function with table driven inputs are as below:

Benchmarks Without any code changes: With assembly code changes With native Go code changes
Tan/2^(-1)-8 10.17 ns/op 10.23 ns/op 13.53 ns/op
Tan/2^(1)-8 10.02 ns/op 10.16 ns/op 13.16 ns/op
Tan/2^(10)-8 9.852 ns/op 10.11 ns/op 13.32 ns/op
Tan/2^(20)-8 10.62 ns/op 10.69 ns/op 12.77 ns/op
Tan/2^(40)-8 10.25 ns/op 10.76 ns/op 13.42 ns/op
Tan/2^(50)-8 10.69 ns/op 10.89 ns/op 13.16 ns/op
Tan/2^(51)-8 10.38 ns/op 10.52 ns/op 13.55 ns/op
Tan/2^(52)-8 2.505 ns/op 31.63 ns/op 40.26 ns/op
Tan/2^(60)-8 2.507 ns/op 33.48 ns/op 40.60 ns/op
Tan/2^(80)-8 2.520 ns/op 31.57 ns/op 42.28 ns/op
Tan/2^(100)-8 2.503 ns/op 31.94 ns/op 40.70 ns/op
Tan/2^(200)-8 2.617 ns/op 27.24 ns/op 37.57 ns/op
Tan/2^(400)-8 2.809 ns/op 28.51 ns/op 37.43 ns/op
Tan/2^(-10)-8 10.13 ns/op 10.48 ns/op 13.25 ns/op
Tan/2^(-15)-8 10.17 ns/op 10.29 ns/op 12.52 ns/op
Tan/2^(-20)-8 10.26 ns/op 10.53 ns/op 13.54 ns/op
Tan/3^(-10)-8 10.07 ns/op 10.18 ns/op 12.25 ns/op
Tan/3^(-15)-8 10.14 ns/op 10.31 ns/op 12.51 ns/op

From the above table, please note that the time taken for all the inputs beyond "2^51" till "2^400" is just around "2ns" (as shown in the first column) with the existing code, because the Tan function was only returning a "NaN" value as soon as the control enters it. No logic was getting executed and hence no time was spent in the function.

@gopherbot
Copy link

Change https://go.dev/cl/470595 mentions this issue: math: support huge arguments in Tan on s390x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Vishwanatha-HD Vishwanatha-HD

Labels
arch-s390x Issues solely affecting the s390x architecture. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@cagedmantis @mknyszek @bmkessler @gopherbot @mundaym @Vishwanatha-HD