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

Remove usage of -DDISABLE_OPENACC (GPU build simplification) #2653

Merged
merged 6 commits into from
Jan 27, 2024

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented Jan 8, 2024

  • always use Random123 via unified memory support
  • some of the cpp files were avoiding it via -DDISABLE_OPENACC but with the addition of RANDOM construct, the Random123 streams are allocated during model setup and hence should be allocated via unified memory. So it's better to do this in one way everywhere (unless there is performance degradation).

See Olli's comment:

OL210813: this shouldn't be needed anymore, but it may have a small performance benefit

For the setup functions (under io/*) and two artificial cells, I don't think there can be a performance impact.

CI_BRANCHES:NMODL_BRANCH=master,

- always use Random123 via unified memory support
- some of the cpp files were avoidit it via -DDISABLE_OPENACC
  but with the addition of RANDOM construct, the Random123 streams
  are allocated during model setup. So it's better to done this
  in one way unless there is performance degradation.
@pramodk pramodk requested a review from iomaganaris January 8, 2024 16:07
@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 2fd11a1 -> Azure artifacts URL

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a0f1848) 66.19% compared to head (2fd11a1) 66.18%.

❗ Current head 2fd11a1 differs from pull request most recent head 47e79ba. Consider uploading reports for the commit 47e79ba to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2653      +/-   ##
==========================================
- Coverage   66.19%   66.18%   -0.02%     
==========================================
  Files         559      559              
  Lines      108937   108939       +2     
==========================================
- Hits        72109    72098      -11     
- Misses      36828    36841      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

Copy link
Member

@iomaganaris iomaganaris left a comment

Choose a reason for hiding this comment

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

I must say I'm not familiar why we disabled OpenACC with those files but I don't see any issue enabling OpenACC/Unified memory.
Have you tested this branch with corenrn-perf CI in case there is any performance degradation?

@pramodk
Copy link
Member Author

pramodk commented Jan 9, 2024

Have you tested this branch with corenrn-perf CI in case there is any performance degradation?

@iomaganaris. Yes - https://bluebrainproject.slack.com/archives/C032L6U24MB/p1704732829835149

I must say I'm not familiar why we disabled OpenACC with those files but I don't see any issue enabling OpenACC/Unified memory.

Here is the background:

Hence the initial introduction of the -DISABLE_OPENACC macro.

Today, we don't have this constraint - thanks to unified memory-related changes from Olli. But we thought if artificial cells are going to execute on CPU, why allocate those streams using CUDA unified memory? So we kept it as is.

With Michael changes from Random123, we are going to allocate Random123 streams from io/phase2.cpp. And then there was a bit of confusion between where we allocate Random123 streams with CUDA unified memory vs where it's plain malloc/new.

And just to avoid this confusion, I thought why not remove it and make it simple? (NMODL should also remove DISABLE_OPENACC but that could be separate update)

pramodk added a commit to BlueBrain/nmodl that referenced this pull request Jan 9, 2024
pramodk added a commit to BlueBrain/nmodl that referenced this pull request Jan 9, 2024
pramodk added a commit that referenced this pull request Jan 9, 2024
squashed merge of #2653

commit 2fd11a1
Author: Pramod Kumbhar <[email protected]>
Date:   Mon Jan 8 22:26:20 2024 +0100

    revert part of the commit: unified memory when CORENEURON_ENABLE_GPU is ON

commit cb7b3f6
Author: Pramod Kumbhar <[email protected]>
Date:   Mon Jan 8 17:12:04 2024 +0100

    make clang-format happy

commit ff6bf40
Author: Pramod Kumbhar <[email protected]>
Date:   Mon Jan 8 16:59:26 2024 +0100

    Remove usage of -DDISABLE_OPENACC (GPU build simplification)
    - always use Random123 via unified memory support
    - some of the cpp files were avoidit it via -DDISABLE_OPENACC
      but with the addition of RANDOM construct, the Random123 streams
      are allocated during model setup. So it's better to done this
      in one way unless there is performance degradation.
Copy link

✔️ 47e79ba -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ c5a31fd -> Azure artifacts URL

pramodk added a commit to BlueBrain/nmodl that referenced this pull request Jan 27, 2024
* See neuronsimulator/nrn/pull/2653 for details/motivation
* We now always use unified memory for all Random123 usage
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

✔️ e41dd09 -> Azure artifacts URL

@pramodk
Copy link
Member Author

pramodk commented Jan 27, 2024

The result from the coreneuron perf CI pipeline with this neuron + nmodl branches:

image

@pramodk pramodk merged commit c1e245d into master Jan 27, 2024
34 of 35 checks passed
@pramodk pramodk deleted the pramodk/random123-unified-mem-always branch January 27, 2024 12:41
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