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

math: use Abs rather than if x < 0 { x = -x } #21812

Closed
mundaym opened this issue Sep 8, 2017 · 8 comments
Closed

math: use Abs rather than if x < 0 { x = -x } #21812

mundaym opened this issue Sep 8, 2017 · 8 comments

Comments

@mundaym
Copy link
Member

mundaym commented Sep 8, 2017

Now that #13095 is closed we should consider using the Abs function where possible in the math library (and possibly elsewhere in the std lib).

For example, this statement in j0.go could presumably be replaced:

if x < 0 {
    x = -x
}

There are subtle differences between the if statement and Abs (NaN handling for example) but most of the math functions handle those cases explicitly anyway.

We should also be aware that don't fully optimize Abs on all architectures yet, but I think this will come with time. s390x and ppc64x, for example, can do Abs in a single instruction whereas the if statement is more difficult to optimize in the general case (because of NaN/-0.0 inputs mainly).

This could be a good issue for a first time contributor, though be aware that you should include before/after benchmarks and for some functions you may need to manually disable assembly implementations to exercise the Go code.

@mundaym mundaym added this to the Unplanned milestone Sep 8, 2017
@ch3ck
Copy link

ch3ck commented Sep 8, 2017

I'll work on this one. Currently looking through the math library for other instances of this.

@laboger
Copy link
Contributor

laboger commented Sep 11, 2017

I have a change ready to be submitted to optimize the Abs and Copysign for ppc64x, because as you mention above, we have a single instruction for those. Not your if statement however.

@ch3ck
Copy link

ch3ck commented Sep 12, 2017

@laboger I made some changes though using Abs following the suggestions of @mundaym above. You could find the PR here: https://go-review.googlesource.com/c/go/+/63050. I think we could synchronize our efforts to fix this.

@gopherbot
Copy link

Change https://golang.org/cl/63050 mentions this issue: math: Use Abs rather than if x < 0 { x = -x}

@gopherbot
Copy link

Change https://golang.org/cl/63190 mentions this issue: math: Use Abs rather than if x < 0 { x = -x }

@laboger
Copy link
Contributor

laboger commented Oct 2, 2017

Here is my CL to make Abs an intrinsic on ppc64x. https://go-review.googlesource.com/c/go/+/67130

@gopherbot
Copy link

Change https://golang.org/cl/84437 mentions this issue: math: use Abs rather than if x < 0 { x = -x }

@kishtatix
Copy link
Contributor

@golang golang locked and limited conversation to collaborators Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants