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

Initial conversion to const generics #1018

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Feb 26, 2021

This PR builds on top rust-lang/rust#82447 to start transition intrinsics to use const generics. This is the first step towards solving #248.

  • Updated the testing infrastructure to support const generics.
  • Updated the intrinsic verification to support const generics.
  • Ported _mm_shuffle_ps and _mm_prefetch to use const generics.

@rust-highfive
Copy link

@Amanieu: no appropriate reviewer found, use r? to override

@Lokathor
Copy link
Contributor

I can help reviewing this later today.

Copy link
Contributor

@Lokathor Lokathor left a comment

Choose a reason for hiding this comment

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

Seems solid so far, though I don't really know about assert-instr-macro or srdarch-verify at all.

#[rustc_legacy_const_generics(2)]
#[stable(feature = "simd_x86", since = "1.27.0")]
pub unsafe fn _mm_shuffle_ps<const mask: i32>(a: __m128, b: __m128) -> __m128 {
assert!(mask >= 0 && mask <= 255);
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is a breaking change compared to before, where you just discarded bits outside the lowest 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but C compilers already reject out-of-range immediates at compile-time. I would argue that this is a bug fix since we model the vendor's API more closely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the Intel Intrinsics Guide again, I see that it's supposed to be unsigned int imm8, so you're right.

Why did we ever accept an i32 to begin with? I guess this should have been u32 the whole time, and arguably u8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oof, i'm actually already subscribed to this one and i'd forgotten about it. okay then.

Actually since _MM_SHUFFLE is still nightly, maybe we should change the shuffle to correctly take u32 and then change _MM_SHUFFLE (again) to make it also emit u32 values.

But even then, if we're going to panic on values outside of u8, it's hard to justify not just taking a u8 to begin with. I guess, are these conversions to const generics supposed to be otherwise completely non-breaking?

Copy link
Member Author

Choose a reason for hiding this comment

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

rust-lang/rust#82447 will rewrite an expression at the AST level to use const generics, but won't do anything to change the type of that expression. So I don't think we can change the type here, unless we decide to accept the breaking change anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well if the ship has sailed that's ultimately fine I guess.

crates/core_arch/src/x86/sse.rs Show resolved Hide resolved
@@ -101,7 +109,23 @@ fn functions(input: TokenStream, dirs: &[&str]) -> TokenStream {
} else {
quote! { None }
};
let required_const = find_required_const(&f.attrs);

let required_const = find_required_const("rustc_args_required_const", &f.attrs);
Copy link
Member

@lqd lqd Feb 28, 2021

Choose a reason for hiding this comment

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

Won't this ignore the #[rustc_legacy_const_generics] attributes when the verifier checks the required consts, since this empty list will be passed to it here ? (edit: I'll add a fix when updating other intrinsics later today -- done in #1023)

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