Skip to content

Check that target features required by LLVM intrinsics are enabled #3180

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
Merged
merged 1 commit into from
Nov 22, 2023
Merged

Check that target features required by LLVM intrinsics are enabled #3180

merged 1 commit into from
Nov 22, 2023

Conversation

eduardosm
Copy link
Contributor

Fixes #3178

@eduardosm

This comment was marked as resolved.

Sign in to view
RalfJung
RalfJung reviewed Nov 21, 2023
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Implementation looks good, I just hope we got them all. :)

Could you add a test to ensure the UB indeed does get reported properly?

@eduardosm
Copy link
Contributor Author

Oh right, SSE4.1 is enabled by default on macOS

RalfJung
RalfJung reviewed Nov 22, 2023
View reviewed changes
tests/fail/shims/intrinsic_target_feature.rs Outdated

unsafe {
// Pass, since SSE is enabled
let _ = addss(_mm_setzero_ps(), _mm_setzero_ps());
Copy link
Member

Choose a reason for hiding this comment

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

Are these must_use, or why the let _?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply to make it explicit that we don't care about the returned value

Copy link
Member

Choose a reason for hiding this comment

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

I find this more confusing than helpful, TBH. I'd prefer if you removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So be it

@RalfJung
Copy link
Member

Thanks for the PR!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 22, 2023

📌 Commit 14dbb29 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 22, 2023

⌛ Testing commit 14dbb29 with merge cad4f40...

@bors
Copy link
Contributor

bors commented Nov 22, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing cad4f40 to master...

1 similar comment
@bors
Copy link
Contributor

bors commented Nov 22, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing cad4f40 to master...

@bors bors merged commit cad4f40 into rust-lang:master Nov 22, 2023
@eduardosm eduardosm deleted the check-intrinsics-target-feature branch November 22, 2023 18:43
@eduardosm eduardosm mentioned this pull request Mar 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check for required target features when evaluating LLVM intrinsics
3 participants
@eduardosm @RalfJung @bors