Handle a non-Fallback baseline
#105
Conversation
|
Still outstanding is the changelog. I'm planning to leave this until after we get v0.3.0 out the door. |
DJMcNab
force-pushed
the
baseline_level
branch
from
f1b4c27 to
f25fbb4
Compare
There was a problem hiding this comment.
This looks fantastic - and reading through this, I really like it.
As a side note, this is extremely hard to test haha. I ran your script and also poked around the compiled result but I cannot statically determine if all code paths are behaving as expected. However, I think that this is overall a great improvement.
Thank you!
Edit: I am least confident with the x86 changes. Those might be good candidates for a double check from someone else.
| #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] | ||
| #[cfg(all( | ||
| any(target_arch = "x86", target_arch = "x86_64"), | ||
| not(all(target_feature = "avx2", target_feature = "fma")) |
There was a problem hiding this comment.
Just to clarify - this line ensures that if avx2 and fma are not both detected we want to fall through to the Avx2 case?
There was a problem hiding this comment.
This says "if our target doesn't statically support [both avx2 and fma], then we might be on a machine which only has SSE4.2 support. As such, we need to check if the currently detected Level is SSE4.2."
That is, we only need to even support the SSE4.2 level if we don't have support for the better level.
This is more closely documented on Level - I'll add a comment pointing there.
| // This macro turns whether the `force_support_fallback` macro is enabled into a boolean literal | ||
| // in `dispatch`, which allows it to be used correctly cross-crate. | ||
| // This trickery is required because macros are expanded in the context of the calling crate, including for | ||
| // evaluating `cfg`s. |
There was a problem hiding this comment.
This pattern is so nice!
There was a problem hiding this comment.
Yeah - I thought I was going to have to make the macro be truly awful for it to work, but the "inversion of control" paradigm luckily worked perfectly here.
| /// Note that this is unaffected by the `force-support-fallback` feature. | ||
| /// Instead, you should use [`Level::fallback`] if you require the fallback level. | ||
| pub const fn baseline() -> Self { | ||
| // TODO: How do we possibly test that this method works in all cases? |
There was a problem hiding this comment.
Agreed! But from carefully reading through this PR the various cfg cases look correct to me.
There was a problem hiding this comment.
I've added a pointer to check_targets.sh as well.
fearless_simd/src/lib.rs
Outdated
| // TODO: Level::Avx2(avx2) => Some(...) | ||
| #[cfg(not(all(target_feature = "avx2", target_feature = "fma")))] | ||
| Level::Sse4_2(sse42) => Some(sse42), |
There was a problem hiding this comment.
Can you elaborate on the TODO? (just in the comment)
Can it be deleted?
There was a problem hiding this comment.
I've fixed the issue. I didn't want to write a justification here whilst #108 was still in flight, but that's now addressed.
|
LGTM! Great stuff! |
3 participants
Fixes #74
A couple of notes:
simd_dispatchin this PR. As such, I don't want to land this before #simd > v0.3.0sh ./check_targets.shat the root. I want to defer running this in CI; I suspect it would be very expensive.This does not address #92. This does however add a
force_support_fallbackfeature, so it doesn't completely remove the fallback functionality.