Skip to content

Compilation for x86 x86_64 without AVX#39

Merged
vincenthz merged 2 commits intotyped-io:masterfrom
Emurgo:master
Feb 15, 2023
Merged

Compilation for x86 x86_64 without AVX#39
vincenthz merged 2 commits intotyped-io:masterfrom
Emurgo:master

Conversation

@lisicky
Copy link
Copy Markdown
Contributor

@lisicky lisicky commented Dec 26, 2022

Hi! I have faced a problem when compilation for x86 or x86_64 without AVX emits an error. It happens because there isn't a restriction for a compiler not to compile a code with AVX intrinsics when that feature is not available. So I have prepared the fix for that case, and I would appreciate it if you check this PR.

@vincenthz
Copy link
Copy Markdown
Collaborator

Hi, thanks for the contribution, with which compilation flags do you come across this failure ?

The code is playing maybe a bit loose with the definitions of the flags / what they mean, but the intent was to add runtime detection of cpu features (although hit a roadblock on how to do this with stable rust) so that the code is there for all platform (avx, avx2, ...) but only used when it's been detected and have the fastest implementation regardless of cpu compilation flags.

@lisicky
Copy link
Copy Markdown
Contributor Author

lisicky commented Jan 5, 2023

@vincenthz I was trying to compile it for android x86 when I got the error. mod avx2 is always compiled if you compile for x86, even if you do it without AVX2 support.

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
mod avx2;

Anyway #[cfg attribute does all stuff in compile time, and there is a case when you compile the lib with AVX2 support, and if you try to execute that code on a machine with only AVX or without SIMD instructions at all, you will get SIGILL or another error depends on an environment you run on.

Because in the code below, HAS_AVX2 will be compiled as true even if a cpu doesn't have AVX2 instruction set because the current implementation of cryptoxide forces to use AVX and AVX2 features because without it cryptoxide won't compile for x86.

#[cfg(target_feature = "avx2")]
const HAS_AVX2: bool = true;
#[cfg(not(target_feature = "avx2"))]
const HAS_AVX2: bool = false;
if HAS_AVX2 {
return avx2::compress_b(&mut self.h, &mut self.t, buf, last);
}

And my PR is an attempt to prevent the cases I described above. I hope we will reach a consensus on this issue.

@renanvalentin
Copy link
Copy Markdown

I found a similar issue when building a lib using rust-android-gradle:

/cryptoxide-0.4.2/src/hashing/blake2/avx2.rs:59:17
    |
59  |             a = _mm256_add_epi64(a, $m);
    |                 ^^^^^^^^^^^^^^^^ not found in this scope
...
320 |     ROUND!(9, load9!());
    |     ------------------- in this macro invocation
    |
    = note: this error originates in the macro `G` which comes from the expansion of the macro `ROUND` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this function
    |
1   | use core::arch::x86::_mm256_add_epi64;

@lisicky
Copy link
Copy Markdown
Contributor Author

lisicky commented Jan 24, 2023

Hi @vincenthz ! I also made a new PR #40 . Could you check it ? It's important to us to have compatibility with x86 machines.

@vincenthz vincenthz merged commit 0a57815 into typed-io:master Feb 15, 2023
@vincenthz
Copy link
Copy Markdown
Collaborator

I'm not certain this is the right way to go at this stage, but to avoid further delay, this is the safest path right now until I figure really what to do with compilation and autodetection.

I've fiddled with flags to try to reproduce the build error, but to not available. So i'm totally sure that support 32 bits x86 (which it seems that's what it is) is actually useful to anything these days, and not the artifact of a typical default generator of android package targets.

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.

3 participants