Skip to content

Run-time feature detection for AES-NI and TSC #312

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

Merged
merged 8 commits into from
Feb 2, 2018

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Feb 1, 2018

TSC doesn't have an LLVM feature, so IIUC we still can't use simd_test for it (its enabled for all x86 targets). Anyways, offering run-time feature detection for it should do no harm.

@gnzlbg gnzlbg requested a review from alexcrichton February 1, 2018 17:33
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 1, 2018

I wish we would make the clippy and and rustfmt builds fail hardly. Otherwise every time I do a commit and want to format things appropriately I get a lot of noise in other parts of the library. The same applies to clippy: when I want to check that I haven't written something unnecessarily complex: .map(...).unwrap_or vs .map_or, foo.iter() vs &foo, xxx vs xxx_with (I always forget that the _with methods exist...), unnecessary &foos, unnecessary transmutes, etc., etc., etc. I get a lot of noise from everywhere in the library as well.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 1, 2018

So... it seems that the feature cache was full on 32-bit because this PR just added the 32th run-time feature test and I was using an AtomicUsize for the cache...

I have added a debug_assert! for this, which although not perfect, should catch this in the future as long as we test the run-time feature detection system a bit (which we do, against cupid).

I have also increased the size of the cache on 32-bit by making it an AtomicU64. The underlying type of the cache was previously being leaked everywhere, so I've compartmentalized it a bit more, so that in the future increasing the cache again to, for example, be implemented using a pair of AtomicU64s can be done locally.

@gnzlbg gnzlbg force-pushed the aes_runtime branch 2 times, most recently from 58dfd3e to c6eac51 Compare February 1, 2018 18:54
@alexcrichton
Copy link
Member

Hm I think AtomicU64 may not be quite as portable as we might like, but on 32-bit could we use a pair of two AtomicUsize?

@alexcrichton
Copy link
Member

(otherwise though this looks good to me!)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 2, 2018

@alexcrichton Is there a reason to prefer AtomicUsize over AtomicU32 on 32-bit ?

EDIT: this PR is otherwise ready.

@alexcrichton
Copy link
Member

Oh I just tend to prefer stable things to unstable things, but either should work fine!

@alexcrichton alexcrichton merged commit ee046e0 into rust-lang:master Feb 2, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 2, 2018

@alexcrichton we would still the new methods to be const fn for AtomicUsize to work here without unstable features because of the static :/

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.

2 participants