keccak: add asm feature; use cpufeatures on aarch64

[tarcieri]

Gates asm support (added in #23) under a crate feature.

Uses the cpufeatures crate to detect the presence of the sha3 extension for ARMv8 CPUs, automatically falling back to a software implementation if it isn't available.

cc @recmo

[tarcieri]

It's a bit unfortunate this is a hard dependency on aarch64, although I couldn't figure out how to gate it better

[newpavlov]

What about gating on feature or configuration flag inside the cfg statement?

[tarcieri]

This doesn't work:

[target.'cfg(all(target_arch = "aarch64", feature = "asm"))'.dependencies]
cpufeatures = "0.2"

It prints this warning:

warning: Found feature = ... in target.'cfg(...)'.dependencies. This key is not supported for selecting dependencies and will not work as expected. Use the [features] section instead: https://doc.rust-lang.org/cargo/reference/features.html

This however, seems to work:

[target.'cfg(all(keccak_asm, target_arch = "aarch64"))'.dependencies]
cpufeatures = "0.2"

So that's a possible argument for using a cfg attribute for gating rather than a Cargo feature.

[tarcieri]

Should this perhaps be a cfg! attribute rather than a crate feature?

[newpavlov]

Hm, I am not sure. Application developers would probably want to have it enabled by default without any additional steps from users. It's a slightly different situation from the aes crate where configuration flags are used mostly for testing backends.

[tarcieri]

@newpavlov there's not really a way to make this interface work with an init token. Does it seem ok to you?

[newpavlov]

Yeah, it's not ideal, but should be fine. One potential alternative is to expose an unsafe gated f1600_aarch64_asm function and do the switch inside sha3.

[tarcieri]

Sure, that works. I can update the PR.

[newpavlov]

I am not saying a separate function is a better approach. Just floating an idea.

[tarcieri]

It seems nice to have to me, especially for the sha3 use case.

It's unsafe and #[target_feature(enable = "sha3")]-gated, which should prevent casual misuse.

[tarcieri]

After the discussion about crate features vs cfg attributes, I could still go either way. They both have pros and cons.

I guess I'm weakly in favor of a feature just for consistency with how sha1 and sha2 provide an asm feature.

[tarcieri]

8e40212 makes f1600_asm a pub unsafe function with a #[target_feature(enable = "sha3")] attribute, re-exported from the toplevel

[tarcieri]

Gonna merge this.

@newpavlov will wait for your approval for a release

[newpavlov]

I think f1600_asm is too generic. Imagine hypothetical future implementation based on XKCP. Since the function is arch specific, I think it should combine arch name and target feature, something like f1600_arch64_sha3 or f1600_armv8_sha3. Also I am not sure it's worth to define cpufeatures::new!(armv8_sha3_intrinsics, "sha3"); in keccak, sha3 looks like a better place for it.

[tarcieri]

I think f1600_asm is too generic. Imagine hypothetical future implementation based on XKCP.

...but this is the XKCP ASM implementation. I can add target info to the name though. Edit: opened #26.

I think it should combine arch name and target feature, something like f1600_arch64_sha3 or f1600_armv8_sha3

But if you're thinking of having an intrinsics implementation, it would be ambiguous? Hence why to include *_asm in the name.

armv8_sha3 is redundant IMO. There are no other relevant ARMv8 target features to use as intrinsics. We can include it for clarity I guess, but in that case I think it should probably be f1600_armv8_sha3_asm.

[tarcieri]

Also I am not sure it's worth to define cpufeatures::new!(armv8_sha3_intrinsics, "sha3"); in keccak, sha3 looks like a better place for it.

I think it's nice to have the default API optionally accelerated for non-SHA-3 applications of Keccak.

sha3 can also do gating the exact same way, calling into keccak_p as a fallback but with token-based initialization, so having the cpufeatures-based gating doesn't hurt there IMO.

[newpavlov]

But if you're thinking of having an intrinsics implementation, it would be ambiguous?

I don't think so. Ideally, we would use intrinsics here, but since they are currently unstable we rely on asm! instead.

[newpavlov]

sha3 can also do gating the exact same way, calling into keccak_p as a fallback but with token-based initialization, so having the cpufeatures-based gating doesn't hurt there IMO.

The problem is that it makes cpufeatures a hard dependency for keccak on ARM, even though the new function could be in theory used without runtime detection. In other words, this crate does not use any runtime detection by itself, so cpufeatures in it looks out of place to me.

[tarcieri]

That's going to be a problem with sha3 as well. AFAICT the only way to avoid it is switching to a cfg attribute for activation.

[newpavlov]

Yes, but at least sha3 does runtime detection with the feature enabled, so it's a better place to depend on cpufeatures. In future after MSRV bump it will use runtime detection unconditionally on ARM.

[tarcieri]

...at which point the concern about a hard dependency on cpufeatures is moot?

I'm confused what you're concerned about. IMO the main problem now is there's a dependency on cpufeatures even if the asm feature isn't enabled, and that will go away when it's "always on".

The only alternative to using cpufeatures in keccak is having every single downstream crate which consumes it implement the same aarch64-specific cpufeatues gating, which seems wrong to me. Why duplicate that logic on a crate-by-crate basis? It would seem to create quite a maintenance burden if we change anything.

The reason for doing it in sha3 as well is so it can use an init token for a minor efficiency boost we haven't implemented or measured yet.

[newpavlov]

Ah... I thought you've removed runtime detection in f1600 in favor of exposing f1600_armv8_sha3_asm.

My idea was to completely remove runtime detection and let user crates (such as sha3) to decide whether to use runtime or compile-time detection. I am not sure it makes sense to expose the intrinsics-based function, if f1600 uses runtime detection under the hood.

In other words, I am fine with either one of the following options:

• Use runtime detection in f1600 and keep f1600_armv8_sha3_asm and armv8_sha3_intrinsics private.
• Expose f1600_armv8_sha3_asm without armv8_sha3_intrinsics (i.e. keccak will not depend on cpufeatures). Keep f1600 strictly software-based.

But not with a mix of them.

[tarcieri]

The current configuration permits the following:

• f1600 autodetects the sha3 feature but doesn't use an init token
• f1600_armv8_sha3_asm + keccak_p(state, u64::KECCAK_F_ROUND_COUNT) allow downstream crates to select between the optimized hardware implementation and a pure software implementation using an init token

I don't see a disadvantage of this approach, especially with sha3 as the main downstream consumer. Where is the issue?

[newpavlov]

Saying that you should use keccak_p(state, u64::KECCAK_F_ROUND_COUNT) as a software implementation of f1600 is... an obscure solution. It's quite probable that unaware users would try to combine f1600 and f1600_armv8_sha3_asm, which obviously will be sub-optimal. At the very least we should introduce f1600_soft.

[tarcieri]

But the point of having an f1600 with CPU feature autodetection is so those downstream users don't have to care or propagate the autodetection code themselves.

The point of using keccak_p in this case is for an unproven microoptimization in the sha3 crate.

Perhaps we should just punt on exposing it at all and leave all of the feature detection in the keccak crate, and circle back if and when someone demonstrates the performance benefit is worth it.

[newpavlov]

Perhaps we should just punt on exposing it at all and leave all of the feature detection in the keccak crate, and circle back if and when someone demonstrates the performance benefit is worth it.

Yes, it's the first option listed earlier.