-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
cranelift: add i64.{ishl,ushr,ashr} libcalls #1787
Conversation
cc @iximeow |
c5901e3
to
1b20dab
Compare
1b20dab
to
09a78fd
Compare
Subscribe to Label Actioncc @bnjbvr
This issue or pull request has been labeled: "cranelift", "cranelift:module"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
@@ -1376,6 +1378,8 @@ fn convert_ushr( | |||
let mut pos = FuncCursor::new(func).at_inst(inst); | |||
pos.use_srcloc(inst); | |||
|
|||
let isa_flags = isa_settings::Flags::new(&isa.flags(), isa_settings::builder()); |
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 a drive-by comment: I'm not too sure this will work. To get the ISA flags I had to add some extra machinery (see 849d71b)
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.
And it's use: 6ab817e#diff-f5aa2e18f8d875720c00f30b86cb69d4R1523
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.
Yeah, you're right, it doesn't work. Which PR is that machinery added in?
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.
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.
(Oh, sorry, @whitequark, I see you already figured that out below!)
Looks like this PR depends on #1759. |
3dac9b0
to
5b491b8
Compare
@abrown I rebased the PR on master. I think some checks are failing because of unrelated issues. |
I'm confused, isn't master supposed to be always green? |
Yes, but some tasks are nightly, with a recent nightly change invalidating what was otherwise fine. #1818 appears to fix this, try a rebase? edit: lol, I see your most recent changes were also a rebase. "keep rebasing until it works" 😁 |
These libcalls are useful for 32-bit platforms. On x86_32 in particular, commit 4ec16fa added support for legalizing 64-bit shifts through SIMD operations. However, that legalization requires SIMD to be enabled and SSE 4.1 to be supported, which is not acceptable as a hard requirement.
5b491b8
to
e3ece4b
Compare
Rebased |
Looks like it's green now! |
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! |
These libcalls are useful for 32-bit platforms.
On x86_32 in particular, commit 4ec16fa added support for legalizing 64-bit shifts through SIMD operations. However, that legalization requires SIMD to be enabled and SSE 4.1 to be supported, which is not acceptable as a hard requirement.
(Actually, when implementing that commit I didn't yet realize that these shifts ought to be legalized via libcalls rather than expansions most of the time, but the expansions are just fine.)
With this commit it is finally possible to use
cargo run --target i686-unknown-linux-gnu run foo.wasm
without having to add--enable-simd
.