-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
argon2: fold compress_avx2
into an inner function
#444
argon2: fold compress_avx2
into an inner function
#444
Conversation
This changes `Argon2::compress` to contain all AVX2-related logic, so it doesn't bleed into the `Block` type yet (especially since that lacks any AVX2-specific implementation).
Followup to #440 cc @dyc3 @newpavlov |
@@ -66,8 +66,10 @@ impl Block { | |||
unsafe { &mut *(self.0.as_mut_ptr() as *mut [u8; Self::SIZE]) } | |||
} | |||
|
|||
/// NOTE: do not call this directly. It should only be called via | |||
/// `Argon2::compress`. | |||
#[inline(always)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this would be fine as #[inline]
but haven't benchmarked yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK #[inline]
is effectively useless for crate-private functions. You could remove the attribute completely to check whether compiler decides to inline it by itself. But to be on the safe side and to be more robust in the face of future compiler changes, I think it's fine to use #[inline(always)]
.
#[cfg(test)] | ||
mod test { | ||
use super::*; | ||
|
||
#[cfg(target_arch = "x86_64")] | ||
#[test] | ||
fn compress_avx2() { | ||
let mut lhs = Block([0; 128]); | ||
lhs.0[0..7].copy_from_slice(&[0, 0, 0, 2048, 4, 2, 1]); | ||
let rhs = Block([0; 128]); | ||
|
||
let result = Block::compress_soft(&rhs, &lhs); | ||
let result_avx2 = unsafe { Block::compress_avx2(&rhs, &lhs) }; | ||
|
||
assert_eq!(result.0, result_avx2.0); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test feels a bit superfluous as for now the implementation is just Block::compress
with optimizations enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see what you were saying before. This looks good to me.
This changes
Argon2::compress
to contain all AVX2-related logic, so it doesn't bleed into theBlock
type yet (especially since that lacks any AVX2-specific implementation).