[stdlib] implement `bit_ceil` and `bit_floor` #2736

LJ-9801 · 2024-05-18T09:40:31Z

fix #2682

LJ-9801 · 2024-05-18T09:54:00Z

Implementation of bit_ceil is based on c++ bit_ceil

soraros · 2024-05-18T18:18:23Z

Should we include the reference in doc string or comment in the future?

stdlib/src/bit/bit.mojo

LJ-9801 · 2024-05-19T05:07:51Z

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?

laszlokindrat

Nice, thanks! I have a few requests for change, nothing major. This will be an excellent improvement for the bit module!

stdlib/src/bit/bit.mojo

stdlib/test/bit/test_bit.mojo

stdlib/src/bit/bit.mojo

laszlokindrat · 2024-05-20T17:35:50Z

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?

I would prefer to not reference external documentation. Can you please just ensure that the docstrings are (more or less) self explanatory?

soraros · 2024-05-20T22:19:21Z

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

LJ-9801 · 2024-05-21T07:22:59Z

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

soraros · 2024-05-22T23:05:41Z

stdlib/src/bit/bit.mojo

+    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)


Could we simply write this? And similarly in bit_floor.

Suggested change

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))

just fixed it, thank!

Why do you need two select at all?

ohhh my bad i misread your suggestions. I thought you just want to change the ones to 1.

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.

laszlokindrat · 2024-05-22T23:37:37Z

If you think a link is not fitting for the doctoring, maybe we could simply leave a comment.

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?

laszlokindrat

LGTM, thanks!

JoeLoser · 2024-05-22T23:42:23Z

!sync

stdlib/src/bit/bit.mojo

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

LJ-9801 · 2024-05-23T04:40:41Z

@laszlokindrat @JoeLoser mojo crash on test_reversed.mojo? test crashes sometime on my M1 mac as well. Most of the time they pass. see 332c152

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

laszlokindrat · 2024-05-26T11:38:15Z

!sync

modularbot · 2024-05-26T12:28:39Z

✅🟣 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.

modularbot · 2024-05-27T05:54:55Z

Landed in abfdac1! Thank you for your contribution 🎉

[External] [stdlib] implement `bit_ceil` and `bit_floor` fix #2682 Co-authored-by: Jiexiang Liu <80805665+LJ-9801@users.noreply.github.com> Closes #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

[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>

[External] [stdlib] implement `bit_ceil` and `bit_floor` fix #2682 Co-authored-by: Jiexiang Liu <80805665+LJ-9801@users.noreply.github.com> Closes #2736 MODULAR_ORIG_COMMIT_REV_ID: 3599b7ce4aba5d369ffb7e0fc09d1571dfb53f1e

LJ-9801 force-pushed the bit-ceil-floor branch from b8e2caa to 12bff3c Compare May 18, 2024 09:45

soraros reviewed May 18, 2024

View reviewed changes

stdlib/src/bit/bit.mojo Outdated Show resolved Hide resolved

LJ-9801 force-pushed the bit-ceil-floor branch 2 times, most recently from c4ace69 to fcd81d2 Compare May 20, 2024 06:41

LJ-9801 marked this pull request as ready for review May 20, 2024 06:45

LJ-9801 requested a review from a team as a code owner May 20, 2024 06:45

laszlokindrat reviewed May 20, 2024

View reviewed changes

LJ-9801 force-pushed the bit-ceil-floor branch 2 times, most recently from f1d7fd4 to 6a38210 Compare May 21, 2024 07:15

LJ-9801 added 4 commits May 22, 2024 15:47

implement bit ceil

a131ff8

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

add cpp ref and remove line

bd0962d

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

bit floor

a0b1a7b

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

fix tests

d3e05c2

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

LJ-9801 force-pushed the bit-ceil-floor branch from 6a38210 to d3e05c2 Compare May 22, 2024 22:49

soraros reviewed May 22, 2024

View reviewed changes

laszlokindrat assigned laszlokindrat and JoeLoser and unassigned laszlokindrat May 22, 2024

laszlokindrat approved these changes May 22, 2024

View reviewed changes

modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 22, 2024

LJ-9801 force-pushed the bit-ceil-floor branch from 5a08693 to 33d9a43 Compare May 23, 2024 03:17

soraros reviewed May 23, 2024

View reviewed changes

stdlib/src/bit/bit.mojo Outdated Show resolved Hide resolved

LJ-9801 force-pushed the bit-ceil-floor branch from 33d9a43 to cf32553 Compare May 23, 2024 04:11

simplify and add cpp ref

332c152

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

LJ-9801 force-pushed the bit-ceil-floor branch from cf32553 to 332c152 Compare May 23, 2024 04:13

LJ-9801 force-pushed the bit-ceil-floor branch from 231cd7d to 98fed7e Compare May 23, 2024 05:20

fix weird crash

538d807

Signed-off-by: Jiexiang Liu <jiexiangliu114@gmail.com>

LJ-9801 force-pushed the bit-ceil-floor branch from 98fed7e to 538d807 Compare May 23, 2024 05:35

modular-automation bot assigned laszlokindrat May 26, 2024

laszlokindrat unassigned JoeLoser May 26, 2024

modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels May 26, 2024

modularbot closed this May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] implement `bit_ceil` and `bit_floor` #2736

[stdlib] implement `bit_ceil` and `bit_floor` #2736

LJ-9801 commented May 18, 2024

LJ-9801 commented May 18, 2024

soraros commented May 18, 2024

LJ-9801 commented May 19, 2024 •

edited

laszlokindrat left a comment

laszlokindrat commented May 20, 2024

soraros commented May 20, 2024

LJ-9801 commented May 21, 2024

soraros May 22, 2024 •

edited

LJ-9801 May 23, 2024

soraros May 23, 2024

LJ-9801 May 23, 2024 •

edited

LJ-9801 May 23, 2024

laszlokindrat commented May 22, 2024

laszlokindrat left a comment

JoeLoser commented May 22, 2024

LJ-9801 commented May 23, 2024 •

edited

laszlokindrat commented May 26, 2024

modularbot commented May 26, 2024

modularbot commented May 27, 2024

[stdlib] implement bit_ceil and bit_floor #2736

[stdlib] implement bit_ceil and bit_floor #2736

Conversation

LJ-9801 commented May 18, 2024

LJ-9801 commented May 18, 2024

soraros commented May 18, 2024

LJ-9801 commented May 19, 2024 • edited

laszlokindrat left a comment

Choose a reason for hiding this comment

laszlokindrat commented May 20, 2024

soraros commented May 20, 2024

LJ-9801 commented May 21, 2024

soraros May 22, 2024 • edited

Choose a reason for hiding this comment

LJ-9801 May 23, 2024

Choose a reason for hiding this comment

soraros May 23, 2024

Choose a reason for hiding this comment

LJ-9801 May 23, 2024 • edited

Choose a reason for hiding this comment

LJ-9801 May 23, 2024

Choose a reason for hiding this comment

laszlokindrat commented May 22, 2024

laszlokindrat left a comment

Choose a reason for hiding this comment

JoeLoser commented May 22, 2024

LJ-9801 commented May 23, 2024 • edited

laszlokindrat commented May 26, 2024

modularbot commented May 26, 2024

modularbot commented May 27, 2024

[stdlib] implement `bit_ceil` and `bit_floor` #2736

[stdlib] implement `bit_ceil` and `bit_floor` #2736

LJ-9801 commented May 19, 2024 •

edited

soraros May 22, 2024 •

edited

LJ-9801 May 23, 2024 •

edited

LJ-9801 commented May 23, 2024 •

edited