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.

Sign up for GitHub

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

Jump to bottom

Cranelift: Add LibCall::Memcmp #2953

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Cranelift: Add LibCall::Memcmp #2953

merged 3 commits into from
Nov 30, 2021

Conversation

scottmcm
Copy link
Contributor

@scottmcm scottmcm commented May 31, 2021 •
edited
Loading

This adds LibCall::Memcmp and a corresponding FunctionBuilder::call_memcmp, following the existing patterns of memcpy, memset, & memmove. The comment says the enum is "likely to grow" and the memcmp's been in libc since C89, so hopefully this is ok.

I'd like to have this for emitting equality tests of large, simple no-padding types, like [u32; 123].

I don't know who should review this.

@bjorn3
Copy link
Contributor

bjorn3 commented May 31, 2021 •
edited
Loading

Could you also add an emit_small_memcmp to cranelift_frontend?

@scottmcm
Copy link
Contributor Author

@bjorn3 Wow you're fast 🙂

I'm not sure how to emit anything nice in emit_small_memcmp, because it's a big-endian comparison. (I guess that's something to add to the todo list once #1092 happens.)

I could add emit_small_mem_eq and emit_small_mem_ne, if those would be worth it?

@bjorn3
Copy link
Contributor

bjorn3 commented May 31, 2021

I guess you can leave it as is for now.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:module labels May 31, 2021
@bjorn3 bjorn3 mentioned this pull request Jun 1, 2021
Merged
@scottmcm
Cranelift: Add LibCall::Memcmp
c266f7f 
The comment says the enum is "likely to grow" and the function's been in libc since C89, so hopefully this is ok.

I'd like to use it for emitting things like array equality.
@scottmcm
Add FunctionBuilder::emit_small_memory_compare
d55a193 
This takes an IntCC for the comparison to do, though panics for Signed*
since memcmp is an unsigned comparison.  Currently it's most useful for
(Not)Equal, but once big-endian loads are implemented it'll be able to
support the other Unsigned* comparisons nicely on more than just bytes.
cfallin
cfallin approved these changes Nov 29, 2021
View reviewed changes
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This is great, thanks very much!

Just a suggestion for extracting a helper below, but otherwise LGTM.

cranelift/frontend/src/frontend.rs Outdated Show resolved Hide resolved
@cfallin cfallin merged commit 952ef03 into bytecodealliance:main Nov 30, 2021
@scottmcm scottmcm deleted the add-memcmp branch November 30, 2021 02:51
@scottmcm scottmcm mentioned this pull request Dec 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfallin cfallin cfallin approved these changes

Assignees
No one assigned
Labels
cranelift:module cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@scottmcm @bjorn3 @cfallin