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/bits: add Rem32/Rem64 #28970

Closed
TuomLarsen opened this issue Nov 27, 2018 · 10 comments
Closed

math/bits: add Rem32/Rem64 #28970

TuomLarsen opened this issue Nov 27, 2018 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@TuomLarsen
Copy link

TuomLarsen commented Nov 27, 2018

On current master, math/bits.Div{32, 64} panic when y <= hi, i.e. on overflow, as seen in the 979d902.

However, even if the quotient might not fit in uint{32, 64}, the remainder does. Therefore I would like to kindly ask you to reconsider because sometimes getting the remainder of uint{64, 128} and uint{32, 64} is still useful, for example in modular arithmetic.

Perhaps there could be additional functions just for remainders, such as Rem64(hi, lo, y uint64) (rem uint64).

@randall77
Copy link
Contributor

That seems like a reasonable thing to want to have.

I don't think we know how to implement such a thing as an intrinsic. The instruction x86 would use will trap if the quotient is too big, regardless of whether you use the quotient or not.

@TuomLarsen
Copy link
Author

In that case, I personally would very much want to keep math/bits.Div{32, 64} simple and fast (i.e. as it is now) and perhaps include Rem{32, 64} sometime in the future, if desired.

@ALTree ALTree changed the title math/bits remainder math/bits: intrinsified Div now panicking on overflow prevents usage of remainder Nov 27, 2018
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 27, 2018
@ALTree ALTree added this to the Go1.12 milestone Nov 27, 2018
@randall77
Copy link
Contributor

Ok, we can decide if we want Rem also (and how to do it, if we do) for 1.13.

Note that you can do Rem32 yourself using the built-in 64-bit divide.

@randall77 randall77 modified the milestones: Go1.12, Go1.13 Nov 27, 2018
@bmkessler
Copy link
Contributor

For the 64-bit case, I not sure if you can get around the need to issue two division instructions to get the remainder when hi >= y. But an implementation you can use now for that case is:

func Rem64(hi, lo, y uint64) (rem uint64) {
  _, rem := bits.Div64(hi%y, lo, y)
  return rem
}

@randall77
Copy link
Contributor

@bmkessler : Very nice.

@rsc
Copy link
Contributor

rsc commented Nov 28, 2018

Based on comments above, sounds like decision is keep panicking and leave Rem32/Rem64 for Go 1.13.

Assuming here that Div64 is new in this release, which I think it is.

@rsc rsc changed the title math/bits: intrinsified Div now panicking on overflow prevents usage of remainder math/bits: add Rem32/Rem64 Nov 28, 2018
@josharian
Copy link
Contributor

Too late to add new API to 1.13.

@josharian josharian modified the milestones: Go1.13, Go1.14 May 3, 2019
@josharian josharian added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels May 3, 2019
@gopherbot
Copy link

Change https://golang.org/cl/197838 mentions this issue: math/bits: add Rem, Rem32, Rem64

@ALTree ALTree self-assigned this Sep 28, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@ALTree ALTree modified the milestones: Backlog, Go1.14 Oct 11, 2019
@TuomLarsen
Copy link
Author

Thank you!

@gopherbot
Copy link

Change https://golang.org/cl/217127 mentions this issue: doc/go1.14: mention new math/bits functions Rem, Rem32, Rem64

gopherbot pushed a commit that referenced this issue Jan 31, 2020
Updates #28970
Updates #36878

Change-Id: I9676f50516dd5b32bd4e44be136fcb9f43776edd
Reviewed-on: https://go-review.googlesource.com/c/go/+/217127
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Jan 30, 2021
@rsc rsc unassigned ALTree Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants