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

[stdlib] [bugfix] Make memcmp function robust against overflow and provide correct SIMD implementation #2524

Closed
wants to merge 6 commits into from

Conversation

mzaks
Copy link
Contributor

@mzaks mzaks commented May 5, 2024

The bugs are reproduced in the unit tests.

Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
@mzaks mzaks requested a review from a team as a code owner May 5, 2024 10:45
@mzaks mzaks changed the title Fix memcmp function against overflow and proper SIMD implementation [stdlib] Fix memcmp function against overflow and proper SIMD implementation May 5, 2024
@mzaks mzaks changed the title [stdlib] Fix memcmp function against overflow and proper SIMD implementation [stdlib] [bugfix] Make memcmp function robust against overflow and provide correct SIMD implementation May 5, 2024
mzaks added 2 commits May 5, 2024 12:53
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Comment on lines 52 to 55
var smaller = s1i < s2i
var bigger = s1i > s2i
if smaller or bigger:
return -int(smaller) + int(bigger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work?

Suggested change
var smaller = s1i < s2i
var bigger = s1i > s2i
if smaller or bigger:
return -int(smaller) + int(bigger)
if s1i != s2i:
return 1 if s1i > s2i else -1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that is even simpler, thanks!

Copy link
Contributor

@soraros soraros May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do similar things to the SIMD part?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually realised why I did it the way I did before I wanted to make a branchless implementation, but stoped half way :)

  var result = 0
  var i = 0
  while i < count:
    var s1i = s1[i]
    var s2i = s2[i]
    var smaller = s1i < s2i
    var bigger = s1i > s2i
    i += 1 + count * int(smaller or bigger)
    result = -1 * int(smaller) + 1 * int(bigger)
  return result

This would be branchless implementation, is a bit more complex but my guess it should be faster. Anyways let's go with the simple one for now. I will create a gist for myself. When we have benchmarks infrastructure setup we can do such micro optimisations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat relevant reference. I still think we could these to a cmp function in SIMD so you can use as much trick as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soraros you were right regarding simplifying SIMD part, I just pushed an update.

@soraros
Copy link
Contributor

soraros commented May 5, 2024

  • Should we restrict the type to unsigned as well?
    • If we do, it's hardly "mem"-compare anymore, IMO
  • Maybe we could add a lexcmp method on SIMD.

mzaks and others added 3 commits May 5, 2024 13:39
Co-authored-by: soraros <soraros@users.noreply.github.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
@ematejska ematejska added the mojo-repo Tag all issues with this label label May 6, 2024
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thank you for the fixes! Always good to make the low-level functions robust to avoid lingering bugs here.

@JoeLoser JoeLoser added imported-internally Signals that a given pull request has been imported internally. merged-internally Indicates that this pull request has been merged internally labels May 8, 2024
@modularbot
Copy link
Collaborator

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

JoeLoser pushed a commit that referenced this pull request May 8, 2024
…flow and provide correct SIMD implementation (#39463)

[External] [stdlib] [bugfix] Make memcmp function robust against
overflow and provide correct SIMD implementation

The bugs are reproduced in the unit tests.

Co-authored-by: Maxim Zaks <maxim.zaks@gmail.com>
Closes #2524
MODULAR_ORIG_COMMIT_REV_ID: 3a71a8adeca0ad82fbc127bf80d94e19b6f6aec0
@JoeLoser JoeLoser added the merged-externally Merged externally in public mojo repo label May 8, 2024
@JoeLoser JoeLoser closed this May 8, 2024
lsh pushed a commit to lsh/mojo that referenced this pull request May 17, 2024
…flow and provide correct SIMD implementation (#39463)

[External] [stdlib] [bugfix] Make memcmp function robust against
overflow and provide correct SIMD implementation

The bugs are reproduced in the unit tests.

Co-authored-by: Maxim Zaks <maxim.zaks@gmail.com>
Closes modularml#2524
MODULAR_ORIG_COMMIT_REV_ID: 3a71a8adeca0ad82fbc127bf80d94e19b6f6aec0

Signed-off-by: Lukas Hermann <lukashermann28@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally mojo-repo Tag all issues with this label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants