Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Add rudimentary CMake support for Feta #1102

Merged
merged 1 commit into from
May 7, 2020
Merged

Add rudimentary CMake support for Feta #1102

merged 1 commit into from
May 7, 2020

Conversation

brycelelbach
Copy link
Collaborator

No description provided.

dkolsen-pgi
dkolsen-pgi reviewed Apr 15, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
dkolsen-pgi
dkolsen-pgi reviewed Apr 15, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
dkolsen-pgi
dkolsen-pgi reviewed Apr 15, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
dkolsen-pgi
dkolsen-pgi reviewed Apr 15, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@dkolsen-pgi
Copy link
Collaborator

I don't know Cmake, so I can't do a meaningful review of most of the change. But I did spot what looks like several incorrect compiler arguments.

@brycelelbach
Copy link
Collaborator Author

Can everyone please re-review, I've made some updates.

griwes
griwes suggested changes Apr 16, 2020
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/common_variables.cmake Outdated Show resolved Hide resolved
@griwes griwes requested a review from alliepiper April 16, 2020 01:00
griwes
griwes approved these changes Apr 16, 2020
View reviewed changes
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

Looks good to me now. I'll do some sanity checks with my preexisting scripts to see if all's good.

griwes
griwes suggested changes Apr 16, 2020
View reviewed changes
Copy link
Collaborator

@griwes griwes left a comment

Choose a reason for hiding this comment

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

Hang on, my scripts no longer set the CUDA host compiler correctly (and I'm passing it via env variable CXX). Don't merge while I investigate.

griwes
griwes suggested changes Apr 16, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@alliepiper
Copy link
Collaborator

I really dislike the pattern of changing the compiler variables programmatically for the reasons mentioned inline. But since we already do that, I wouldn't hold this PR up over it, we can address that later in the eventual CMake refactor.

The other suggestion about the arch option is a "nice to know" low priority QOL change that can be skipped for now. I assume most of these hacks are temporary until we get proper CMake support for Feta, so if it works for feta and doesn't break existing usecases, I'm ok with merging.

@brycelelbach
Copy link
Collaborator Author

I really dislike the pattern of changing the compiler variables programmatically for the reasons mentioned inline. But since we already do that, I wouldn't hold this PR up over it, we can address that later in the eventual CMake refactor.

Until CMake has first class support for Feta, we probably can't avoid this.

@brycelelbach brycelelbach added this to the 1.9.10 milestone Apr 30, 2020
brycelelbach
brycelelbach commented May 6, 2020
View reviewed changes
CMakeLists.txt Outdated
@@ -404,6 +480,11 @@ target_include_directories(
PRIVATE ${PROJECT_SOURCE_DIR}/testing
)

if ("Feta" STREQUAL "${CMAKE_CUDA_COMPILER_ID}")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to only take this branch if the device backend is CUDA.

@brycelelbach brycelelbach force-pushed the feature/feta-cmake-support branch 2 times, most recently from e5daaa4 to d359d02 Compare May 6, 2020 22:41
@brycelelbach
* Add rudimentary CMake support for Feta.
a2a85bf 
* Refactor RDC handling to not create separate explicit targets.
* Drive-by: Don't use the NVCC version of `normal_distribution_base` for
  Feta because it uses `erfcinv`, a non-standard function that Feta doesn't
  have.

Reviewed-by: Allison Vacanti <[email protected]>
Reviewed-by: Michał 'Griwes' Dominiak <[email protected]>
Reviewed-by: David Olsen <[email protected]>
@brycelelbach brycelelbach merged commit 3a83c55 into master May 7, 2020
@brycelelbach brycelelbach deleted the feature/feta-cmake-support branch May 7, 2020 00:21
@brycelelbach brycelelbach restored the feature/feta-cmake-support branch May 16, 2020 06:57
@brycelelbach brycelelbach deleted the feature/feta-cmake-support branch May 16, 2020 07:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alliepiper alliepiper alliepiper approved these changes

@dkolsen-pgi dkolsen-pgi dkolsen-pgi approved these changes

@griwes griwes griwes approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@brycelelbach @dkolsen-pgi @alliepiper @griwes