Change multiversioning control from features to cfgs

[Shnatsel]

An alternative to #258

The problem with features is that they apply to the entire dependency tree, and anything in the dependency tree can enable them. So if you have a library in the dependency tree that uses fearless_simd, it either uses fearless_simd with default features on and locks all users in all their SIMD code into the full set of features, or uses it with default-features=false and then whoever uses simply cargo add on that library doesn't get any SIMD acceleration by default. Both options are bad.

cfg values passed in RUSTFLAGS are global and avoid this issue entirely, letting whoever builds the final binary adjust the build if they know what their target hardware is. It's still possible to pass different cfgs for different crates, so I had to move all cfg logic from macros (that expand into user crate) to logic inside fearless_simd crate so that different parts of the code don't end up in an inconsistent state.

There is some overlap with -C target-cpu=, but this cfg mechanism allows two things that -C target-cpu= doesn't:

1. Binaries with disabled cfgs retain the scalar fallback, and still run on older hardware instead of crashing
2. cfgs allow disabling newer instruction sets like AVX-512 if you know you're targeting e.g. AVX2 only

---

This PR also contains an unrelated fix: Level::Sse4_2 variant would be absent in builds that have a higher baseline, breaking certain kernel! usage as well as any user code that matched on it. This was not the case for Avx2 and Avx512 variants.

[LaurenzV]

Hmm, I'm not sure I'm super convinced about this one, doesn't this mean that we don't have any way of setting a reasonable default inside of vello itself, but instead need to rely on the end user itself to always provide the flags at compile time to ensure they don't get AVX-512 by default? I think this would be a bit unfortunate...

Like, I get that it's also unfortunate that features will be unified across the whole dependency chain so as long as there is one crate that doesn't disable AVX-512 explicitly every other one will get it as well (tbh, maybe let's make AVX-512 opt-in by default, i.e. remove it from the features activated by default? Given that it's still pretty niche in average consumer CPUs, I think this would make sense for now), but I think it would still be useful if we had a way of making this the default in vello. I think vello is a bit special here because we have a lot of code generic over SIMD, in many other cases where you only have a few specialized kernel functions it's probably not that critical to be able to reduce the number of levels. But I really would like there a way to make AVX-512 disabled by default in vello, given the already horrible compile times on x86. (If that is already possible and I'm just misunderstanding this PR, let me know)

[Shnatsel]

But I really would like there a way to make AVX-512 disabled by default in vello, given the already horrible compile times on x86.

Performance vs build time vs binary size is a trade-off that's highly dependent on the application. I don't think this is something that a library should dictate. For development of fearless_simd itself, where iteration time matters, these knobs are quite sufficient. And outside that it really should be left up to the library user.

I would sympathize if Vello showed no improvement from AVX-512, but the end-to-end benchmarks improve by 15% even on hardware without native 512-bit vectors, likely more on hardware with it. AVX-512 is already available in 20% of the hardware in the Steam survey, and this number is set to only grow with all future Intel consumer CPUs starting later next year are slated to have AVX-512 as well.

[Shnatsel]

To be clear, you still can make AVX-512 opt-in via features using this mechanism, I just don't think you should.

[DJMcNab]

This PR also contains an unrelated fix: Level::Sse4_2 variant would be absent in builds that have a higher baseline, breaking certain kernel! usage as well as any user code that matched on it. This was not the case for Avx2 and Avx512 variants.

This is a bug when adding AVX512, not a bug in SSE4.2
The idea is that you don't need to generate the specialised sse4.2 code when the baseline level is already higher than that.
Perhaps this pr also handles this correctly in dispatch; but I don't have time to read the code now.

It isn't clear what the "certain kernel usage" is, but users definitely shouldn't be manually matching the variants; they need to use the as_sse4_2, etc. methods.

Otherwise, I'm in favour of this in theory.

[Shnatsel]

The idea is that you don't need to generate the specialised sse4.2 code when the baseline level is already higher than that.
Perhaps this pr also handles this correctly in dispatch;

Yes, I have verified that it does. I have also tried writing regression tests for it, but they turned out so arcane and confusing that I didn't include them in this PR.

It isn't clear what the "certain kernel usage" is, but users definitely shouldn't be manually matching the variants

Should the enum variants be made private then?

There is a distinction between "I have AVX2 available" and "AVX2 is the highest level available" that can be useful to know about. I guess this can be emulated with level.as_avx2().is_some() && level.as_avx512.is_none(). There are future compatibility issues if you want to check for exactly AVX-512, since there's no level above it now, but I guess there are also future compatibility issues in allowing to match on the exact level? This is out of scope of this PR, but something we might potentially want to address.

The token downgrade pattern specifically is much more natural with public Level variants.

[Shnatsel]

@LaurenzV I'd appreciate a code review now that Daniel has approved the basic direction.

I believe the token downgrade pattern should be sufficient if you prefer to make AVX-512 a non-default Cargo feature in Vello, you just need to annotate the token-downgrading wrapper as #[cfg(not(feature = avx512))] and a wrapper that doesn't downgrade the token with #[cfg(feature = avx512)].

[DJMcNab]

This is great!

Is it potentially even somehow non-breaking, as compared to the most recent release? I haven't fully thought it through, but it seems possible to me.

[DJMcNab]

It isn't clear to me why Neon is different to x86 here?

[Shnatsel]

Isn't not, the code is identical.

I tried to add a comment specifically to that effect (no multiversioning needed due to the hardware, but we keep the same code) but it was clearly confusing so I'll just remove it.

[DJMcNab]

What I mean is, it seems like we should also have a disable_dispatch_neon feature for the platforms which don't require neon.

[Shnatsel]

We don't have any runtime dispatch for NEON, so there is nothing to disable.

The call to is_aarch64_feature_detected! is technically present, but it's gated on #[cfg(target_arch = "aarch64")] where NEON is always available. All the other code dealing with NEON is also gated on #[cfg(target_arch = "aarch64")].

On 32-bit ARM runtime dispatch could be beneficial but we cannot do that because is_arm_feature_detected! is nighly-only with no clear path to stabilization, because the underlying querying mechanisms are very problematic.

[DJMcNab]

I really like this method!

[DJMcNab]

Would it be worthwhile porting this to cfg_select at this point in time? I certainly haven't reviewed the actual cfg conditions carefully.

[Shnatsel]

Not sure, I can take a closer look once our MSRV is above 1.95

I don't expect that to happen soon since I'd like to be conservative with the MSRV. fearless_simd has the potential to become a very foundational crate and some prospective users, e.g. jpeg-encoder used by image crate, are concerned about MSRV: vstroebel/jpeg-encoder#25 (comment)

[DJMcNab]

I do think that 1.85 means we could re-evaluate our MSRV policy. I'd recommend making a Zulip thread if you think that's important.

Certainly this improvement isn't good enough to justify an MSRV bump by itself.

[Shnatsel]

I think the ideal policy wrt MSRV is to ship 1.0, bump minor version for MSRV bumps, and backport security fixes to versions with earlier MSRV. This gets us the best of both worlds.

[Shnatsel]

Yes, I believe this is a non-breaking change. The primary reason I'm going with v0.6 for the upcoming release is the AVX-512 addition, which increases maximum vector width and might break some variable-width code that assumed 256 bits is the maximum.

[Shnatsel]

I believe there are no outstanding action items from the review. Am I clear to push the merge button?

[LaurenzV]

If Daniel approved LGTM