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

Move llvm.x86.* shims into shims::x86 and implement _addcarry_u32 and _subborrow_u{32,64} #3075

Merged
merged 5 commits into from Sep 25, 2023

Conversation

eduardosm
Copy link
Contributor

This PR moves all llvm.x86.* shims into shims::x86 and adds llvm.x86.addcarry.32, llvm.x86.subborrow.32 and llvm.x86.subborrow.64.

Additionally, it fixes the input carry semantics of llvm.x86.addcarry.32. The input carry is an 8-bit value that is interpreted as 1 when it is non-zero.

https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/addcarry-u32-addcarry-u64.html

@bors
Copy link
Collaborator

bors commented Sep 23, 2023

☔ The latest upstream changes (presumably #3072) made this pull request unmergeable. Please resolve the merge conflicts.

{
let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
this.yield_active_thread();
}
"llvm.aarch64.isb" if this.tcx.sess.target.arch == "aarch64" => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"llvm.aarch64.isb" if this.tcx.sess.target.arch == "aarch64" => {
// FIXME: Move these to an `arm` submodule.
"llvm.aarch64.isb" if this.tcx.sess.target.arch == "aarch64" => {

src/shims/x86/mod.rs Show resolved Hide resolved
pub fn main() {
assert_eq!(adc(0, 1, 1), (0, 2));
assert_eq!(adc(1, 1, 1), (0, 3));
assert_eq!(adc(2, 1, 1), (0, 3));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(adc(2, 1, 1), (0, 3));
assert_eq!(adc(2, 1, 1), (0, 3)); // any non-zero carry acts as 1!

@@ -1,3 +1,51 @@
#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
Copy link
Member

Choose a reason for hiding this comment

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

Ah, here's another way to handle tests that should run only on x86. Might be good to also port the other tests (that use the long ignore- list) to this pattern.

Copy link
Contributor Author

@eduardosm eduardosm Sep 23, 2023

Choose a reason for hiding this comment

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

For SSE and SSE2 that will work fine because they are enabled by default. But other features require -C target-feature=+feature, which will generate warnings on non-x86.

Maybe a new directive could be added to compiletest ui_test? Something like @only-x86-family

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've done it for SSE and SSE2 for the time being.

@saethlin
Copy link
Member

It looks to me like you addressed Ralf's comments above but GitHub doesn't seem to have noticed 🤔

@saethlin
Copy link
Member

@eduardosm Can you squash the commits down a bit? I think your original 4 commits are a good organization, but the rest don't stand on their own.

Then this LGTM (and I would rather like to sync these changes into rust-lang/rust).

@eduardosm
Copy link
Contributor Author

Ready!

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 25, 2023

📌 Commit ce3a919 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 25, 2023

⌛ Testing commit ce3a919 with merge 79a9a71...

@bors
Copy link
Collaborator

bors commented Sep 25, 2023

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

@bors bors merged commit 79a9a71 into rust-lang:master Sep 25, 2023
8 checks passed
@eduardosm eduardosm deleted the x86-addcarry-subborrow branch September 25, 2023 16:32
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