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

Allow single and double precision libraries in a single build. #70

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

marsdeno
Copy link
Contributor

Behaviour controlled by ENABLE_SINGLE_PRECISION and ENABLE_DOUBLE_PRECISION.

Behaviour controlled by ENABLE_SINGLE_PRECISION and ENABLE_DOUBLE_PRECISION.
@marsdeno marsdeno requested a review from awnawab February 18, 2025 22:36
@FussyDuck
Copy link

FussyDuck commented Feb 18, 2025

CLA assistant check
All committers have signed the CLA.

@marsdeno marsdeno marked this pull request as draft February 18, 2025 22:38
@wdeconinck
Copy link
Collaborator

In principle OK, but I did not think this was needed. What's the rationale?

@wdeconinck
Copy link
Collaborator

wdeconinck commented Mar 12, 2025

Regardless of my previous comment if we should even do this, and assuming we do:

We have following lines in the CMakelists.txt

get_filename_component( ecwam-source_dir ${CMAKE_CURRENT_SOURCE_DIR} NAME )
if( ecwam-source_dir MATCHES "ecwam_(dp|sp)" )
  set( ECWAM_PROJECT_NAME ${ecwam-source_dir} )
endif()

For backwards compatibility perhaps there we should make sure that if the ecwam-source_dir is "ecwam_dp" it is only enabling double precision, and if it is "ecwam_sp" it should only enable single precision.

The target names currently set to ${ecwam}_${prec} could in this case also be wrongly named as ecwam_dp_dp and ecwam_sp_sp and the logic needs to be adapted. The value of ${ecwam} is set in line 9 of src/ecwam/CMakeLists.txt

@marsdeno
Copy link
Contributor Author

I don't think it is needed at the moment, but it would be a step towards allowing both single and double precision libraries to be linked in a single binary, for example OOPS with single precision trajectory and double precision inner loops.

@wdeconinck
Copy link
Collaborator

I don't think it is needed at the moment, but it would be a step towards allowing both single and double precision libraries to be linked in a single binary, for example OOPS with single precision trajectory and double precision inner loops.

This will then need to go the ectrans route of trying to make the symbols unique.

@awnawab
Copy link
Contributor

awnawab commented Mar 17, 2025

I thought this was needed for easybuild recipes, where we don't want to do the symlinked projects like we do in the bundle but still wish to retain the ability to build both sp and dp targets in a single "bundle" invocation.

In that case, the work of making the symbols unique like in ecTrans could be done in a follow-up PR?

In terms of getting the current PR over the line, we would need a few fixes (which I am happy to help with @marsdeno if you are pressed for time):

  • Only build program targets if HAVE_${prec}
  • Rework bundle logic now that we are not building one precision at a time
  • Rework testing setup now that we are not building one precision at a time

@wdeconinck
Copy link
Collaborator

In that case, the work of making the symbols unique like in ecTrans could be done in a follow-up PR?

Yes a separate PR for sure. We should aim to be backwards compatible still for the time being, i.e. not introducing API changes or ABI changes.

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.

4 participants