Skip to content

miri: implement some llvm.x86.sse.* intrinsics and add tests #113932

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

Closed
wants to merge 1 commit into from
Closed

miri: implement some llvm.x86.sse.* intrinsics and add tests #113932

wants to merge 1 commit into from

Conversation

eduardosm
Copy link
Contributor

Implements LLVM intrisics needed to run most SSE functions from core::arch::x86{,_64}.

Also adds miri tests for those functions (mostly copied from core_arch tests).

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 21, 2023

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung
Copy link
Member

Thanks for the PR!

As a procedural matter, PRs that only change Miri are preferred to be submitted against the Miri repo, https://github.com/rust-lang/miri/. But if porting the PR over is too hard I'm also okay with reviewing it here.

Miri has so far deliberately not implemented these intrinsics. It was just considered too much work. But I guess now you just did it. :) So that's quite cool.

Regarding the review -- to me, cvtss2si looks like the output of a password generator. ;) So what I am saying is, there is no way to deduce from the name of the intrinsic their behavior, that would be obvious to me. Could you add links to documentation that explains the behavior implemented by these intrinsics? I'm generally somewhat concerned that the sse.rs file has basically 0 comments, even when there are very particular things going on (like I see some i32::MIN default values).

) {
i64::MIN
} else {
cvt.value as i64
Copy link
Member

Choose a reason for hiding this comment

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

as casts are dangerous and if you had made this a Miri PR, CI would have failed due to a clippy check. We always use checked casts to make sure this doesn't overflow.

@@ -928,6 +928,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
this.write_scalar(Scalar::from_f64(res), dest)?;
}

"llvm.prefetch" => {
let [_, rw, loc, ty] =
Copy link
Member

Choose a reason for hiding this comment

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

It is never okay to throw away an argument with _. At the least some to_<int type> method should be called on it to ensure it has the right size.

@eduardosm
Copy link
Contributor Author

Thanks for the feedback! I will open the PR in the miri repo.

@eduardosm eduardosm closed this Jul 22, 2023
@eduardosm eduardosm deleted the miri-x86-intrinsics branch August 9, 2023 19:18
bors added a commit to rust-lang/miri that referenced this pull request Aug 11, 2023
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang/rust#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Aug 11, 2023
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang/rust#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
miri: implement some `llvm.x86.sse.*` intrinsics and add tests

PR moved from rust-lang/rust#113932.

Implements LLVM intrisics needed to run most SSE functions from `core::arch::x86{,_64}`.

Also adds miri tests for those functions (mostly copied from core_arch tests).

r? `@RalfJung`

The first commit is the same that the commit in the PR I had opened in the Rust repository. I addressed review comments in additional commits to make it easier to review. I also fixed formatting and clippy warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants