Cray workaround removal #700

abbotts · 2024-11-06T22:18:48Z

Description

This PR removes the CRAY_ACC_WAR workaround code that we put in to deal with CCE not handling acc declare create on allocatable module arrays used in device subroutines. Some additional but more localized workarounds were required to make this removal work completely, but they are less offensive than the CRAY_ACC_WAR code and integrated directly.

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

Scope

This PR comprises a set of related changes with a common goal

If you cannot check the above box, please split your PR into multiple PRs that each have a common goal.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also list any relevant details for your test configuration

Ran the full ./mfc.sh test --gpu test suite on an HPE internal cluster mirroring Frontier's software environment
Ran the full ./mfc.sh test --gpu on Frontier proper
Ran ./mfc.sh test on ARM MacBook
Ran benchmark problem on Frontier with GPU
Ran benchmark problem on Frontier with CPU

Test Configuration:

What computers and compilers did you use to test this:
Frontier with CCE 18.0.0
HPE internal machine with CCE 18.0.0

Checklist

I have added comments for the new code
I added Doxygen docstrings to the new code
I have made corresponding changes to the documentation (docs/)
I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected
I have added example cases in examples/ that demonstrate my new feature performing as expected.
They run to completion and demonstrate "interesting physics"
I ran ./mfc.sh format before committing my code
New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled
This PR does not introduce any repeated code (it follows the DRY principle)
I cannot think of a way to condense this code and reduce any introduced additional line count

If your code changes any code source files (anything in `src/simulation`)

To make sure the code is performing as expected on GPU devices, I have:

Checked that the code compiles using NVHPC compilers
Checked that the code compiles using CRAY compilers
Ran the code on either V100, A100, or H100 GPUs and ensured the new feature performed as expected (the GPU results match the CPU results)
Ran the code on MI200+ GPUs and ensure the new features performed as expected (the GPU results match the CPU results)
Enclosed the new feature via nvtx ranges so that they can be identified in profiles
Ran a Nsight Systems profile using ./mfc.sh run XXXX --gpu -t simulation --nsys, and have attached the output file (.nsys-rep) and plain text results to this PR
Ran an Omniperf profile using ./mfc.sh run XXXX --gpu -t simulation --omniperf, and have attached the output file and plain text results to this PR.
Ran my code using various numbers of different GPUs (1, 2, and 8, for example) in parallel and made sure that the results scale similarly to what happens if you run without the new code/feature

Some of the CCE specific compiler directives are going to hang around. Mark those with _CRAYFTN instead of a new macro.

Removing this procedure pointer is necessary due to a CCE bug, but has the potential to be a breaking change. Other similar changes will be made elsewhere but this specific infrastructure is only being used in a private branch, so isolating it here to avoid confusion.

The main thrust of this commit factors out certian procedure pointers that are causing a bug in the Cray compiler that ignores all `acc declare` statements. This is annoying workaround but not catastrophic given that procedure pointers could be refactored into dispatch subroutines easily and runtime polymorpism was minimal. There are a few other assorted tweaks in the non-CRAY_ACC_WAR logic. WARNING: more complicated boundary conditions are broken in this commit! The next commit will fix them.

…ace CCE bug For some reason CCE OpenACC is failing to keep these specific variables from colliding between m_cbc and m_riemann_solvers. Every other variable type seems fine, but some implicit link must be causing a collision. The trivial bug ugly workaround is rename the variables to "_l" local versions to avoid the collision. If we can ever figure out and fix this bug we could undo this commit.

Somehow missed these the first time around.

This removes all the hacks for the critical CCE bug around allocatable module arrays in ACC routines. Other more localized hacks for CCE bugs remains in the code, but the majority of the source developed for other machines should work on Frontier now.

abbotts · 2024-11-07T17:13:54Z

It seems pretty clear I took something out that was required for the NVHPC builds. Seeing if I can build locally for A100 to figure out what. Right now I'm suspecting it was acc enter data directives for globals, but that confuses me a bit because I though NVIDIA supported automatic device allocation on acc declare create arrays.

codecov · 2024-11-07T17:38:18Z

Codecov Report

Attention: Patch coverage is 29.29293% with 70 lines in your changes missing coverage. Please review.

Project coverage is 42.84%. Comparing base (39e410a) to head (ab4a3c4).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simulation/m_cbc.fpp	3.12%	62 Missing ⚠️
src/common/m_phase_change.fpp	0.00%	3 Missing ⚠️
src/common/m_variables_conversion.fpp	75.00%	1 Missing and 1 partial ⚠️
src/simulation/m_data_output.fpp	83.33%	0 Missing and 1 partial ⚠️
src/simulation/m_rhs.fpp	0.00%	1 Missing ⚠️
src/simulation/m_riemann_solvers.fpp	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #700      +/-   ##
==========================================
- Coverage   43.04%   42.84%   -0.20%     
==========================================
  Files          61       61              
  Lines       16062    16281     +219     
  Branches     1798     1891      +93     
==========================================
+ Hits         6914     6976      +62     
- Misses       8158     8260     +102     
- Partials      990     1045      +55

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

I was too agressive pulling these out. Why it didn't break CCE is a puzzle..

abbotts · 2024-11-07T20:47:33Z

It seems pretty clear I took something out that was required for the NVHPC builds. Seeing if I can build locally for A100 to figure out what. Right now I'm suspecting it was acc enter data directives for globals, but that confuses me a bit because I though NVIDIA supported automatic device allocation on acc declare create arrays.

It was acc enter/exit data directives for regular ALLOCATE. Not sure why CCE didn't fail on this. Going to run tests on Frontier again, but this should all work now. I'll resolve the merge soon.

sbryngelson · 2024-11-07T21:52:45Z

It seems pretty clear I took something out that was required for the NVHPC builds. Seeing if I can build locally for A100 to figure out what. Right now I'm suspecting it was acc enter data directives for globals, but that confuses me a bit because I though NVIDIA supported automatic device allocation on acc declare create arrays.

It was acc enter/exit data directives for regular ALLOCATE. Not sure why CCE didn't fail on this. Going to run tests on Frontier again, but this should all work now. I'll resolve the merge soon.

Thanks, @abbotts!

src/common/m_variables_conversion.fpp

src/simulation/m_cbc.fpp

sbryngelson · 2024-11-08T02:16:50Z

src/simulation/m_data_output.fpp

+
+        integer, intent(in) :: t_step
+
+        if (parallel_io .neqv. .true.) then


.not. parallel_io perhaps?

sbryngelson · 2024-11-08T02:17:59Z

src/simulation/m_start_up.fpp

@@ -1353,10 +1353,8 @@ contains
        ! Associate pointers for serial or parallel I/O
        if (parallel_io .neqv. .true.) then
            s_read_data_files => s_read_serial_data_files


can we just wrap all of this logic in a subroutine instead of only the write subroutine? (include the read one as well)

sbryngelson

thanks so much! i had a question or two, but mostly just figured that we can make the logic in the code a bit more consistent with a couple more additional changes.

sbryngelson · 2024-11-08T14:56:31Z

Will merge once CI runs.

abbotts · 2024-11-08T17:24:27Z

@sbryngelson , I'm not sure why the GA Tech CPU bench run failed. Looking at the logs the PR and Master benchmarks both built and ran fine, but then the master summary yaml file generation failed with:

runner-4/_work/MFC/MFC/master/build/benchmarks/a3bf/hypo_hll.yaml -c phoenix -n 4 -- --gbpp 1

slurmstepd: error: *** JOB 1857066 ON atl1-1-02-002-27-1 CANCELLED AT 2024-11-08T12:04:19 DUE TO PREEMPTION ***
---------------------------------------
Begin Slurm Epilog: Nov-08-2024 12:04:20
Job ID:        1857066
Array Job ID:  _4294967294
User ID:       sbryngelson3
Account:       gts-sbryngelson3
Job name:      shb-bench-cpu
Resources:     cpu=24,mem=48G,node=1
Rsrc Used:     cput=2-03:10:24,vmem=0,walltime=02:07:56,mem=12549932K,energy_used=0
Partition:     cpu-small
QOS:           embers
Nodes:         atl1-1-02-002-27-1
---------------------------------------

abbotts · 2024-11-08T17:26:59Z

Actually, the hypo_hll.out file from the master benchmark run looks like it was killed before it reached the end out output.

abbotts added 12 commits November 6, 2024 16:07

[cray] supress annoying CCE compiler messages

9600777

[cray] Replace CRAY_ACC_WAR with _CRAYFTN when appropriate

df7fb2b

Some of the CCE specific compiler directives are going to hang around. Mark those with _CRAYFTN instead of a new macro.

Remove double acc declared Re

9297d51

[cray] Move a few more CCE directives to _CRAYFTN

28f10b5

Somehow missed these the first time around.

[cray] remove workaround flags from CCE build line

71c689d

clean up import statements that aren't part of abstract interfaces

6b4ce43

[cleanup] ran mfc.sh format pass

7975c3f

[cray] removed one import too many. put one back

d389bf9

abbotts marked this pull request as ready for review November 7, 2024 15:42

abbotts requested review from sbryngelson and henryleberre as code owners November 7, 2024 15:42

[bugfix] remove stray pound responsible for ifort failures

c09b6e1

[cray/bugfix] put acc enter/exit data back into ALLOCATE

eda5bb2

I was too agressive pulling these out. Why it didn't break CCE is a puzzle..

Merge branch 'master' into cray_workaround_removal

0246cb0

sbryngelson reviewed Nov 8, 2024

View reviewed changes

src/common/m_variables_conversion.fpp Show resolved Hide resolved

sbryngelson reviewed Nov 8, 2024

View reviewed changes

src/simulation/m_cbc.fpp Show resolved Hide resolved

sbryngelson reviewed Nov 8, 2024

View reviewed changes

sbryngelson requested changes Nov 8, 2024

View reviewed changes

Address PR reivew comments

ab4a3c4

sbryngelson approved these changes Nov 8, 2024

View reviewed changes

sbryngelson merged commit 1e27e8b into MFlowCode:master Nov 8, 2024
21 of 23 checks passed

sbryngelson mentioned this pull request Nov 9, 2024

Moving Levelset Calculation to Pre-Process #662

Merged

18 tasks

max-Hawkins pushed a commit to max-Hawkins/MFC that referenced this pull request Nov 11, 2024

Cray workaround removal (MFlowCode#700)

6d39682

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cray workaround removal #700

Cray workaround removal #700

abbotts commented Nov 6, 2024 •

edited

Loading

abbotts commented Nov 7, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

abbotts commented Nov 7, 2024

sbryngelson commented Nov 7, 2024

sbryngelson Nov 8, 2024

sbryngelson Nov 8, 2024

sbryngelson left a comment

sbryngelson commented Nov 8, 2024

abbotts commented Nov 8, 2024

abbotts commented Nov 8, 2024


		integer, intent(in) :: t_step

		if (parallel_io .neqv. .true.) then

Cray workaround removal #700

Cray workaround removal #700

Conversation

abbotts commented Nov 6, 2024 • edited Loading

Description

Type of change

Scope

How Has This Been Tested?

Checklist

If your code changes any code source files (anything in src/simulation)

abbotts commented Nov 7, 2024

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

abbotts commented Nov 7, 2024

sbryngelson commented Nov 7, 2024

sbryngelson Nov 8, 2024

Choose a reason for hiding this comment

sbryngelson Nov 8, 2024

Choose a reason for hiding this comment

sbryngelson left a comment

Choose a reason for hiding this comment

sbryngelson commented Nov 8, 2024

abbotts commented Nov 8, 2024

abbotts commented Nov 8, 2024

abbotts commented Nov 6, 2024 •

edited

Loading

If your code changes any code source files (anything in `src/simulation`)

codecov bot commented Nov 7, 2024 •

edited

Loading