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 · 9 comments
Closed

math/bits: add Rem32/Rem64 #28970

TuomLarsen opened this issue Nov 27, 2018 · 9 comments
Assignees
Labels
Milestone

Comments

@TuomLarsen
Copy link

@TuomLarsen 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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Author

@TuomLarsen TuomLarsen commented Nov 27, 2018

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 this to the Go1.12 milestone Nov 27, 2018
@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Contributor

@bmkessler bmkessler commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Nov 27, 2018

@bmkessler : Very nice.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc 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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented May 3, 2019

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 and removed NeedsDecision labels May 3, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 28, 2019

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
@gopherbot gopherbot closed this in 57c63e0 Oct 18, 2019
@TuomLarsen

This comment has been minimized.

Copy link
Author

@TuomLarsen TuomLarsen commented Oct 22, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.