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

BUGFIX: add ARB_CUDA flag to example catalogue #2039

Merged
merged 4 commits into from
Nov 17, 2022
Merged

BUGFIX: add ARB_CUDA flag to example catalogue #2039

merged 4 commits into from
Nov 17, 2022

Conversation

boeschf
Copy link
Contributor

@boeschf boeschf commented Nov 17, 2022

  • adding this flag manually follows the same strategy as the dummy catalogue in unit tests

AdhocMan
AdhocMan reviewed Nov 17, 2022
View reviewed changes
example/ornstein_uhlenbeck/CMakeLists.txt Outdated
@@ -6,6 +6,10 @@ make_catalogue_standalone(
CXX_FLAGS_TARGET ${ARB_CXX_FLAGS_TARGET_FULL}
VERBOSE ON)

if(ARB_WITH_NVCC)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also ARB_WITH_CUDA_CLANG and ARB_WITH_HIP_CLANG to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, but then the tests should also handle this, and they do not - what is the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

For HIP, we need to define ARB_HIP and ARB_CUDA for the other two. See https://github.com/arbor-sim/arbor/blob/master/CMakeLists.txt#L369

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right. Changes:

  • use make_catalogue instead of make_catalogue_standalone
  • link to arbor-private-deps
  • mark all files as c++ sources when using cuda-clang or hip-clang

@boeschf boeschf requested a review from AdhocMan November 17, 2022 11:33
AdhocMan
AdhocMan requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@AdhocMan AdhocMan 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 apart from that one comment. Maybe also use bors to run tests including GPU, if that's still working.

@boeschf boeschf requested a review from AdhocMan November 17, 2022 12:05
@boeschf
Copy link
Contributor Author

boeschf commented Nov 17, 2022

bors try

bors bot added a commit that referenced this pull request Nov 17, 2022
@bors
Try #2039:
7467db2
AdhocMan
AdhocMan approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@AdhocMan AdhocMan left a comment

Choose a reason for hiding this comment

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

Not sure how to check if bors is still running, but this lgtm otherwise.

@boeschf boeschf merged commit ff3494e into arbor-sim:master Nov 17, 2022
@bors
Copy link

bors bot commented Nov 17, 2022

try

Timed out.

1 similar comment
@bors
Copy link

bors bot commented Nov 20, 2022

try

Timed out.

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

@AdhocMan AdhocMan AdhocMan approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@boeschf @AdhocMan