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

Implement llvm.ctpop.v* intrinsics #3072

Merged
merged 1 commit into from Sep 23, 2023
Merged

Conversation

eduardosm
Copy link
Contributor

Tested through x86 avx512vpopcntdq and avx512bitalg functions.

I picked them from #2057 (comment), which looked easy.

Tested through x86 avx512vpopcntdq and avx512bitalg functions.
let (op, op_len) = this.operand_to_simd(op)?;
let (dest, dest_len) = this.place_to_simd(dest)?;

assert_eq!(dest_len, op_len);

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We generally have the policy that ICEing when intrinsics are used incorrectly is fine. I think we should apply that to LLVM intrinsics as well. If rust-lang/rust#116093 lands, people will get an appropriate warning when enabling the feature.

@saethlin
Copy link
Member

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Sep 23, 2023

📌 Commit f50f062 has been approved by saethlin

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 23, 2023

⌛ Testing commit f50f062 with merge bd5d841...

@bors
Copy link
Collaborator

bors commented Sep 23, 2023

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing bd5d841 to master...

@bors bors merged commit bd5d841 into rust-lang:master Sep 23, 2023
8 checks passed
Copy link
Member

Choose a reason for hiding this comment

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

These test files are getting a bit too fine-grained IMO. What about just using intrinsics-x86-other for "everything except the SSE/SSE2 intrinsics"? llvm.x86.addcarry.64 should also be tested there, probably.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, we already have tests/pass/intrinsics-x86.rs. So this could be added there?

Copy link
Member

Choose a reason for hiding this comment

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

The tests are grouped by what CPU features they use, and the avx512 CPU features are significantly more fine-grained than the others. I wouldn't mind lumping all the avx512 intrinsics together, but I don't think they belong in "other" with llvm.x86.rdtsc.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's fair. But the two test files added by this PR should IMO be merged.

@eduardosm can you wile a follow-up PR?

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 will!

@eduardosm eduardosm deleted the llvm.ctpop branch September 23, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants