-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[stdlib] implement bit_ceil
and bit_floor
#2736
Conversation
Implementation of |
Should we include the reference in doc string or comment in the future? |
@soraros ops yeah you are right. I'll include it in the docstring for now. @laszlokindrat which one do you think is more suitable? |
c4ace69
to
fcd81d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! I have a few requests for change, nothing major. This will be an excellent improvement for the bit
module!
I would prefer to not reference external documentation. Can you please just ensure that the docstrings are (more or less) self explanatory? |
@laszlokindrat My idea was that performant numerical code can contain a large amount of black magic, thus not very self-explanatory. If we are indeed using an existing implementation (from a paper, libc++, etc) and include a reference to them, it would be less surprising to future maintainers when they see the said magic. If you think a link is not fitting for the doctoring, maybe we could simply leave a comment. |
f1d7fd4
to
6a38210
Compare
@laszlokindrat I have fixed all the requests. In terms of the reference, I do agree with @soraros in that numerical code does sometime contains "black magic". While these two functions seems pretty ok to understand, there might be functions that has wizardry implementation in the future and adding a reference can definitely help future maintainer. I have taken out the c++ reference. If there is a consent, I can put it back in. |
Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>
Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>
stdlib/src/bit/bit.mojo
Outdated
alias ones = SIMD[type, simd_width].splat(1) | ||
|
||
var less_then_one = (val <= ones).select(ones, val) | ||
|
||
return (val > ones).select(1 << bit_width(val - ones), less_then_one) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simply write this? And similarly in bit_floor
.
alias ones = SIMD[type, simd_width].splat(1) | |
var less_then_one = (val <= ones).select(ones, val) | |
return (val > ones).select(1 << bit_width(val - ones), less_then_one) | |
return (val <= 1).select(1, 1 << bit_width(val - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just fixed it, thank!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need two select
at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh my bad i misread your suggestions. I thought you just want to change the ones
to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the added line seems to cause a crash in the reverse test sometime, I still kept the ones
and did
(val > 1).select(1 << bit_width(val - ones), ones)
to avoid the crashing.
Yeah, a link within a comment in the implementation is fine, since that's a message to developers working on the stdlib. I just didn't want the generated docs to reference external links (and also, docstrings should explain what the function does, not how). @LJ-9801 could you please do this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
!sync |
Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>
@laszlokindrat @JoeLoser mojo crash on |
Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>
!sync |
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
Landed in abfdac1! Thank you for your contribution 🎉 |
[External] [stdlib] implement `bit_ceil` and `bit_floor` fix modularml#2682 Co-authored-by: Jiexiang Liu <80805665+LJ-9801@users.noreply.github.com> Closes modularml#2736 MODULAR_ORIG_COMMIT_REV_ID: 3599b7ce4aba5d369ffb7e0fc09d1571dfb53f1e
[External] [stdlib] implement `bit_ceil` and `bit_floor` fix modularml#2682 Co-authored-by: Jiexiang Liu <80805665+LJ-9801@users.noreply.github.com> Closes modularml#2736 MODULAR_ORIG_COMMIT_REV_ID: 3599b7ce4aba5d369ffb7e0fc09d1571dfb53f1e Signed-off-by: Avinag <udayagiriavinag@gmail.com>
fix #2682