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.

Sign up for GitHub

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

Jump to bottom

Use FFLAGS for PARPACK with autotools and mpi #449

Closed
wants to merge 1 commit into from
Closed

Use FFLAGS for PARPACK with autotools and mpi #449

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions PARPACK/SRC/MPI/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
AUTOMAKE_OPTIONS = subdir-objects # Needed as debug/stat* are not in current directory.

F77 = $(MPIF77)
FFLAGS_SAV = @FFLAGS@
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sylvestre: OK for you? Was this intentional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this was intentional. I missed it is still used by libparpack@LIBSUFFIX@@ITF64SUFFIX@_la_FFLAGS = $(FFLAGS_SAV) which can now use FFLAGS instead. Or keep it to a one line change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libparpack@LIBSUFFIX@@ITF64SUFFIX@_la_FFLAGS = $(FFLAGS_SAV) which can now use FFLAGS instead.

I guess this is a good move.

Or keep it to a one line change?

@sylvestre: what do you think? Was this FFLAGS_SAV intentional (bug fix?) back in the old days?!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, is there anything interesting in the git blame/log ? :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@turboencabulator: git-blame is on you! :)
Why did you need FFLAGS_SAV? Bug fix? If yes, which one? Can you review this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem was the no-optimization stuff trying to fix?

Don't know!
@loqs and @turboencabulator: I let you converge to the best solution to PR. Let me know when the PR is ready to be merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loqs: is there the same problem with cmake? I guess no.
Otherwise feel free to PR the equivalent fix at cmake side.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

2. What `FFLAGS` are you trying to add?  Is it something that `configure` should be detecting and adding to `AM_FFLAGS`?

FFLAGS=-march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -fstack-clash-protection -fcf-protection -ffile-prefix-map=/build/arpack/src=/usr/src/debug/arpack
I detected -fcf-protection was missing from PARPACK.

Copy link
Author

@loqs loqs Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@loqs: is there the same problem with cmake? I guess no. Otherwise feel free to PR the equivalent fix at cmake side.

autotools only. cmake is building pdlamch10.f and pslamch10.f with optimization

arpack-ng/CMakeLists.txt

Line 385 in ba440d1

file(GLOB parpacksrc_STAT_SRCS ${arpack_SOURCE_DIR}/dbgini.f ${arpack_SOURCE_DIR}/staini.f ${arpack_SOURCE_DIR}/PARPACK/SRC/MPI/*.f)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking your FFLAGS might be related to MPI, but doesn't look like it.

Usually when I see -O0 it's because the code has some obscure bug that the optimizer exposes. But these two files seem fairly simple, and if CMake is building them optimized and nobody has complained yet... I say we remove the no-optimization stuff.

FFLAGS =

PSRC = pcontext.f
SSRC = psnaitr.f psnapps.f psnaup2.f psnaupd.f psneigh.f psneupd.f psngets.f \
Expand Down
Loading