Skip to content
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

Propagate SIMD flags into sub-makes #5598

Closed
solardiz opened this issue Dec 1, 2024 · 3 comments
Closed

Propagate SIMD flags into sub-makes #5598

solardiz opened this issue Dec 1, 2024 · 3 comments

Comments

@solardiz
Copy link
Member

solardiz commented Dec 1, 2024

As noticed in #5593, the CFLAGS we use to build the .a libraries in subdirectories do not include the main build's SIMD flags. This is non-optimal at least in that it doesn't enable usage of the same CPU features, and potentially worse - it may cause slow transitions between VEX-encoded and legacy SSE code.

@magnumripper
Copy link
Member

magnumripper commented Dec 1, 2024

The reason for @CFLAGS@ not being "it" is probably because of historical confusion about john.o special treatment, for CPU fallback.

Current top Makefile.in

CPPFLAGS = @CPPFLAGS@
CFLAGSX = -c @CFLAGS@ @JOHN_NO_SIMD@ @CFLAGS_EXTRA@ @OPENSSL_CFLAGS@ @OPENMP_CFLAGS@ @HAVE_MPI@ @PTHREAD_CFLAGS@ $(CPPFLAGS)
# CFLAGS for use on the main john.c file only
CFLAGS_MAIN = -DAC_BUILT @CC_MAIN_CPU@ $(CFLAGSX)
CFLAGS = -DAC_BUILT @CC_CPU@ @CC_MAIN_CPU@ $(CFLAGSX)

We could rework that and edit configure.ac so @CFLAGS@ is all needed, but the easiest fix for the subdirs' Makefile.in's is probably to standardize on something like:

CFLAGS = -c -DAC_BUILT @CC_CPU@ @CC_MAIN_CPU@ @CFLAGS@ @JOHN_NO_SIMD@ @CFLAGS_EXTRA@ @OPENSSL_CFLAGS@ @OPENMP_CFLAGS@ @HAVE_MPI@ @PTHREAD_CFLAGS@ @CPPFLAGS@

For #5593 I think we should amend configure.ac so the -maes -mpclmul is added to @CC_CPU@, so no change (other than the above) needed in mbedtls/Makefile.in for that.

@magnumripper magnumripper self-assigned this Dec 1, 2024
@magnumripper
Copy link
Member

magnumripper commented Dec 1, 2024

@solardiz I intend to fix this issue and #5593 while completely ignoring any performance regression [from using intrinsics instead of asm] on some CPU or the other (which could certainly be cared for, but later/separately). OK?

@solardiz
Copy link
Member Author

solardiz commented Dec 1, 2024

fix this issue and #5593 while completely ignoring any performance regression [from using intrinsics instead of asm] on some CPU or the other (which could certainly be cared for, but later/separately). OK?

Yes, that's OK with me. Thank you!

magnumripper added a commit to magnumripper/john that referenced this issue Dec 2, 2024
This commit only applies to autoconf builds.

Closes openwall#5598
magnumripper added a commit to magnumripper/john that referenced this issue Dec 2, 2024
This commit only applies to autoconf builds.

Closes openwall#5598
claudioandre-br pushed a commit to claudioandre-br/JohnTheRipper that referenced this issue Dec 2, 2024
This commit only applies to autoconf builds.

Closes openwall#5598
magnumripper added a commit to magnumripper/john that referenced this issue Dec 3, 2024
This commit only applies to autoconf builds.

Closes openwall#5598
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants