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

Configure renamings #866

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Configure renamings #866

wants to merge 13 commits into from

Conversation

waynemitchell
Copy link
Contributor

This gives more generic names to several configuration variables related to the GPU, which previously had cuda-related names. This should help to clarify and unify configuration options across different GPU architectures.

rfalgout
rfalgout approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@rfalgout rfalgout left a comment

Choose a reason for hiding this comment

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

This looks good to me. My only concern is how this will affect our user's build systems. We might want to ask a few groups here to take this branch for a quick test drive to see if it breaks anything.

@waynemitchell waynemitchell linked an issue Mar 21, 2023 that may be closed by this pull request
Configure CUCC/CUFLAGS need to be updated for SYCL and HIP #830
Open
victorapm
victorapm approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@victorapm victorapm left a comment

Choose a reason for hiding this comment

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

Nice work! I left a couple of comments, but they are minor

src/CMakeLists.txt Show resolved Hide resolved
src/config/Makefile.config.in Outdated Show resolved Hide resolved
@victorapm
Copy link
Contributor

Hi all, this is a note that we need to update this line of hypre's spack package:

https://github.com/spack/spack/blob/f49e9591b7702268ddc28ce6cae81da4e0b6a4fc/var/spack/repos/builtin/packages/hypre/package.py#L247

It should be --with-extra-GPUFLAGS after this PR.

oseikuffuor1
oseikuffuor1 approved these changes Apr 6, 2023
View reviewed changes
Copy link
Contributor

@oseikuffuor1 oseikuffuor1 left a comment

Choose a reason for hiding this comment

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

I agree with @victorapm's suggestion for HYPRE_GPU_ARCH, otherwise everything looks good to me. Thanks.

@victorapm
Copy link
Contributor

Hi all, PETSc will also need to be updated after this change, see:

https://gitlab.com/petsc/petsc/-/blob/main/config/BuildSystem/config/packages/hypre.py#L130

ulrikeyang
ulrikeyang approved these changes Jun 12, 2023
View reviewed changes
Copy link
Contributor

@ulrikeyang ulrikeyang left a comment

Choose a reason for hiding this comment

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

Overall, this looks good. My main concern is - like others have already commented - that we need to make sure that this doesn't break users' builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ulrikeyang ulrikeyang ulrikeyang approved these changes

@rfalgout rfalgout rfalgout approved these changes

@oseikuffuor1 oseikuffuor1 oseikuffuor1 approved these changes

@victorapm victorapm victorapm approved these changes

@liruipeng liruipeng Awaiting requested review from liruipeng

@osborn9 osborn9 Awaiting requested review from osborn9

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Configure CUCC/CUFLAGS need to be updated for SYCL and HIP
5 participants
@waynemitchell @victorapm @ulrikeyang @rfalgout @oseikuffuor1