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.

Sign up for GitHub

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

cmd/compile: ppc64x rounding large uint64 to float64 cast incorrectly #15539

Closed
mundaym opened this issue May 4, 2016 · 6 comments
Closed

cmd/compile: ppc64x rounding large uint64 to float64 cast incorrectly #15539

mundaym opened this issue May 4, 2016 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@mundaym
Copy link
Member

mundaym commented May 4, 2016

Seen in TestFP/TestFp on the builder: https://build.golang.org/log/d9a463f06b55a67d70805a7bcb1cd18eed65d3ec

uint64 to float64 casts should round to nearest with ties to even. However on ppc64 large uint64 values are shifted right by 1 so that they can be reinterpreted as int64 values (so that they can be fed into the fcfid instruction). Unfortunately this truncates the original value, possibly (as in this test I think) resulting in a tie where there was none before.

Can ppc64 use the fcfidu instruction instead?
@mundaym
Copy link
Member Author

mundaym commented May 4, 2016

cc @laboger @dr2chase

@gopherbot
Copy link

CL https://golang.org/cl/22772 mentions this issue.

@bradfitz bradfitz added this to the Go1.7 milestone May 4, 2016
@minux minux reopened this May 5, 2016
@minux
Copy link
Member

minux commented May 5, 2016 via email

@mundaym
Copy link
Member Author

mundaym commented May 5, 2016

It probably doesn't since as far as I can tell fcfidu was only added in 2010... One option is to use fcfid for every type except uint64. Then for uint64 we could use fcfidu when in little endian mode (as you suggested in the CL) and use bitwise/integer instructions to do the cast manually in big endian mode (or something more clever).

I don't really like the idea of keeping the code the way it is on big endian machines. If a big endian builder is added it won't pass the tests.

@cherrymui cherrymui mentioned this issue May 5, 2016
Closed
@mundaym
Copy link
Member Author

mundaym commented May 5, 2016

Based on Lynn and Russ' comments in golang-dev and in the CL I'm going to close this issue again for now.

@mundaym mundaym closed this as completed May 5, 2016
@gopherbot
Copy link

CL https://golang.org/cl/24191 mentions this issue.

gopherbot pushed a commit that referenced this issue Jun 17, 2016
@mundaym
cmd/compile: use power5 instructions for uint64 to float casts
f3d5478 
Use the same technique as mips64 for these casts (CL 22835).

We could use the FCFIDU instruction for ppc64le however it seems
better to keep it the same as ppc64 for now.

Updates #15539, updates #16004.

Change-Id: I550680e485327568bf3238c4615a6cc8de6438d7
Reviewed-on: https://go-review.googlesource.com/24191
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Michael Munday <munday@ca.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Jun 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
4 participants
@bradfitz @minux @gopherbot @mundaym