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 x86 AVX2 intrinsics #3492

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

eduardosm
Copy link
Contributor

No description provided.

@eduardosm eduardosm force-pushed the intrinsics-x86-avx2 branch from 964ade8 to 80316e1 Compare April 19, 2024 19:03
@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2024

My hope was that we'd implement them in libstd as fallback bodies instead (#3397), but I guess now that we have a shim impl and tests, doing that would be simpler as we have some help against messing up the fallback bodies

src/shims/x86/mod.rs Outdated Show resolved Hide resolved
src/shims/x86/mod.rs Show resolved Hide resolved
src/shims/x86/mod.rs Outdated Show resolved Hide resolved
src/shims/x86/avx2.rs Show resolved Hide resolved
tests/pass/intrinsics-x86-avx2.rs Show resolved Hide resolved
@eduardosm eduardosm force-pushed the intrinsics-x86-avx2 branch from 80316e1 to af9fc5d Compare April 23, 2024 17:07
@oli-obk
Copy link
Contributor

oli-obk commented Apr 24, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 24, 2024

📌 Commit a79b1f1 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 24, 2024

⌛ Testing commit a79b1f1 with merge 9d6623e...

@bors
Copy link
Contributor

bors commented Apr 24, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 9d6623e to master...

@bors bors merged commit 9d6623e into rust-lang:master Apr 24, 2024
8 checks passed
@eduardosm eduardosm deleted the intrinsics-x86-avx2 branch April 24, 2024 16:35
let dest = this.project_index(&dest, i)?;

// Converting to a host "i128" works since the input is always signed.
let res = op.to_int(dest.layout.size)?.unsigned_abs();
Copy link
Member

Choose a reason for hiding this comment

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

This may be worth an assertion.

Alternatively, a moire clear implementation might avoid using host integers entirely and just use binop_with_overflow.

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'm not sure what to check with an assertion. How would you implement this with binop_with_overflow?

Copy link
Member

@RalfJung RalfJung Apr 27, 2024

Choose a reason for hiding this comment

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

The assertion would check that the element type is signed.

How would you implement this with binop_with_overflow?

Sorry, I meant wrapping_binary_op.

Basically run the equivalent of if op < 0 { -op } else { op }. It's one BinOp::lt and one... okay you need wrapping_unary_op as well, UnOp::Neg. You can use ImmTy::from_int to create a 0 of the right type.

Ok(())
}

fn pack_generic<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

A doc comment would be good.

let (mask, mask_len) = this.operand_to_simd(mask)?;
let (dest, dest_len) = this.mplace_to_simd(dest)?;

// There are cases like dest: i32x4, offsets: i64x2
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the offsets are longer than the dest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extra elements are ignored.

Copy link
Member

Choose a reason for hiding this comment

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

And that's how the intrinsic is intended to work? Wow.

Would be good to have a comment explaining that.

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.

4 participants