feat: use feature [compress, asm] of sha2 for aarch64

[dgbo]

Hi,

As of now, if FIL_PROOFS_USE_MULTICORE_SDR == 1, sha2::compress256 is used to hash the data in Phase 1.
The function sha2::compress256 is provided with feature compress of sha2.
But for aarch64, only with feature compress, the sha2::compress256 does not use the aarch64 accelerators, e.g. sha256su0, but degradats to soft_compress.
According to our tests, that ~x6 slower than the ASM one.
We should add feature asm to enable those aarch64 accelerators as the code [1] in sha2 shows.

Due to the compile BUG of sha2 fixed by PR https://github.com/RustCrypto/hashes/pull/207, commited as 78f00c5f6d218, we need a git dependency here.

In addition, to keep x86 use only compress of sha2 as before, we need add -Zfeatures=itarget to support conditional compilation of dependency feature based on target.
I've raise a separate PR (filecoin-project/filecoin-ffi#163) in filecoin-ffi for this.

Regards.

[1] https://github.com/RustCrypto/hashes/blob/a18e0bdaac45ad97419292b24fb1e3d470336d11/sha2/src/sha256.rs#L145

[vmx]

It looks like the recent 0.9.3 release contains that commit, so you can now point to that version instead of a Git commit.

[dgbo]

Thank you for the review.

I guess we don't need -Zfeatures=itarget [1] either, since the sha2-0.9.3 has updated to use the instrinsic backend for x86 platforms even if asm is enbled [2].
Use sha2-0.9.3 with feature ["compress", "asm"] would be enough, I will withdraw [1] if this PR can be integerated.

Regards.

[1] filecoin-project/filecoin-ffi#163
[2] RustCrypto/hashes#224

[dgbo]

Hi, can I get a review for this?

Regards.

[cryptonemo]

@dgbo Thanks, this looks good! Are you able to rebase it against master?

[dgbo]

@dgbo Thanks, this looks good! Are you able to rebase it against master?

Thank you! I've rebased this against commit 35ca38a of master in the newest version.

[dignifiedquire]

We should not enable "asm" by default, this will result in build issues and compatibility accross other platforms

[cryptonemo]

Ah, good catch. Thanks!

[dgbo]

Thanks for the comments.

I run local tests with lotus-bench with this PR on AArch64 and AMD7742 to reproduce the issues as mentioned.
Lotus builds on both platforms are fine. On AMD7742, the x86 instrinsic backend, i.e. sha2::sha256::x86::compress, is used as before.

Could you please be more specific about the build and compatibility issues? I must miss something. :-）

[dignifiedquire]

The issue is that the default flags are used to generate prebuilts, which sometimes end up not being portable anymore when asm is enabled by default. So instead this should be threaded through as a general asm feature and only be enabled in filecoin-ffi in the native/local builds.

[dgbo]

filecoin-ffi

Sorry for being silent last few days, it's Spring festival.

Thanks for clarifying this.
I see we have some platform dependent code in this repository, e.g. prefetch [1].
How do you think if we only enable asm feature for aarch64 here? Something like:

## rust-fil-proofs changes
diff --git a/storage-proofs/porep/Cargo.toml b/storage-proofs/porep/Cargo.toml
index e52f30d..f6549c4 100644
+[target."cfg(target_arch = \"aarch64\")".dependencies]
+sha2 = { version = "0.9.3", features = ["compress", "asm"] }
+[target."cfg(not(target_arch = \"aarch64\"))".dependencies]
+sha2 = { version = "0.9.3", features = ["compress"] }

##  filecoin-ffi changes
diff --git a/install-filcrypto b/install-filcrypto
index dac54b7..dc56a4d 100755
--- a/install-filcrypto
+++ b/install-filcrypto
@@ -150,12 +150,13 @@ build_from_source() {
     # Add feature specific rust flags as needed here.
+    additional_flags="-Zfeatures=itarget"
     if [ "${FFI_USE_BLST_PORTABLE}" == "1" ]; then
-        additional_flags="--no-default-features --features blst --features blst-portable"
+        additional_flags="--no-default-features --features blst --features blst-portable ${additional_flags}"
     elif [ "${FFI_USE_BLST}" == "1" ]; then
-        additional_flags="--no-default-features --features blst"
+        additional_flags="--no-default-features --features blst ${additional_flags}"
     else
-        additional_flags="--no-default-features --features pairing"
+        additional_flags="--no-default-features --features pairing ${additional_flags}"
     fi

[1]

rust-fil-proofs/storage-proofs-porep/src/stacked/vanilla/macros.rs

Line 47 in 64390b6

macro_rules! prefetch {

[dignifiedquire]

yes I believe something like that should work

[dgbo]

OK, updated a commit. Only enable asm feature for aarch64 now.
Reopened filecoin-project/filecoin-ffi#163 in filecoin-ffi for the changes we discussed, which supports conditional compilation of dependency feature based on target.
Both PRs have been rebased against master.

Suggestions?
Thanks. :-)

[dgbo]

Oops, this PR seems broken when I was trying to rebase it. Sorry about that. Reopening...

[dignifiedquire]

looks good to me

[dgbo]

@cryptonemo @dignifiedquire Thanks for the review.