Enable GPU execution of atm_compute_vert_imp_coefs via OpenACC #1241

abishekg7 · 2024-10-23T22:15:00Z

Initial OpenACC port of atm_compute_vert_imp_coefs.

Separating the computation of cofrz into a separate parallel clause is required for correctness.
Variables b_tri and c_tri are declared private in lieu of extending the storage to two dimensions, like a_tri.
Back substitution/Computation of alpha_tri and gamma_tri needs to be sequential

mgduda · 2024-11-01T20:41:47Z

@abishekg7 Can you clean up the commits and add a description to this PR?

abishekg7 · 2024-11-06T22:55:41Z

@abishekg7 Can you clean up the commits and add a description to this PR?

Thanks. Done!

gdicker1 · 2025-01-02T22:01:36Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

@@ -1894,32 +1894,46 @@ subroutine atm_compute_vert_imp_coefs_work(nCells, moist_start, moist_end, dts,
      real (kind=RKIND) :: dtseps, c2, qtotal, rcv
      real (kind=RKIND), dimension( nVertLevels ) :: b_tri, c_tri

+      MPAS_ACC_TIMER_START('atm_compute_vert_imp_coefs_work [ACC_data_xfer]')
+      !$acc enter data copyin(rdzw, zz, fzm, fzp, rdzu, cqw, p, t, qtot, rb, &
+      !$acc                   rtb, rt, pb)


Some of the fields here are invariant (across timesteps) and should be handled in the mpas_atm_dynamics_{init,finalize} routines instead.

Generally these invariant fields are those contained in the "invariant stream" (L421-L524 in the atm Registry.xml) or those accessed through the "mesh" mpas_pool (L1222-L1516 in the atm Registry.xml)

Done. thanks for the catch!

gdicker1 · 2025-01-02T22:02:49Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F


      !  set coefficients
      dtseps = .5*dts*(1.+epssm)
      rcv = rgas/(cp-rgas)
      c2 = cp*rcv

+      !$acc parallel
+      !$acc loop


I think it would be better to be explicit with loop directives (e.g. loop gang worker here).

Makes sense. I think I'll pause on this commit until I also am able to profile the code.

Done. No performance penalties for this change, with the limited area case at least.

gdicker1 · 2025-01-02T22:04:08Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+
+      !$acc exit data copyout(cofrz, cofwr, cofwz, coftz, cofwt, a_tri, b_tri, &
+      !$acc                   c_tri, alpha_tri, gamma_tri)
+      !$acc exit data delete(rdzw, zz, fzm, fzp, rdzu, cqw, p, t, qtot, rb, rtb, rt, pb)


Besides handling fields like fzm in mpas_atm_dynamics_finalize instead, these directives should be surrounded by MPAS_ACC_TIMER_{START,STOP} calls.

gdicker1 · 2025-01-02T22:08:31Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

 ! MGD bad to have all threads setting this variable?
      do k=1,nVertLevels
         cofrz(k) = dtseps*rdzw(k)
      end do
-
+      !$acc end parallel 
+


This empty line contains extra spaces that should be removed. Might be good to double check for any trailing whitespace in the lines you're adding.

Done. thanks!

gdicker1

This PR may need a little more work. I'm getting differences in 2 fields in the history file when comparing the regional testcase. Restart files are reported as matching.

Output from compare_netcdf.py:

Comparing files: f1 - f2                                                                                                                             
        f1=../2024Dec31_MPASAPR1223_OpenACC/baseline_acc_1735844488/history.2019-09-01_00.06.00.nc                                                   
        f2=test_att1735695005/history.2019-09-01_00.06.00.nc                                                                                         
                                                                                                                                                     
            Variable  Min       Max                                                                                                                  
=========================================
initial_time is not a numeric field and will not be compared
xtime is not a numeric field and will not be compared
        depv_dt_fric -32.448421  33.315609
     depv_dt_fric_pv -32.448421  30.517973
mminlu is not a numeric field and will not be compared

Some suggested edits as well.

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

mgduda · 2025-01-16T01:17:31Z

@abishekg7 The code changes themselves look good to me now. I'd only request that the commit history be cleaned up. If you want to keep the handling of the time-invariant fields in a separate commit, that seems fine, but just a single commit for this PR seems sufficient.

abishekg7 · 2025-01-16T16:31:07Z

Rebased and squashed to a single commit, and results are still bit identical.

gdicker1 · 2025-01-16T16:49:52Z

@abishekg7, I think everything is looking good except this sentence in the commit message

The parallel clauses now specify a default(present)

This seems like an incomplete thought, like there should be more after "...sent)". You could drop the "a" or replace the sentence with something like:

The parallel constructs use default(present) clauses.

With something about "to enforce data management" potentially being added too.

Aside: As I understand OpenACC terminology, directives refer to the part of the statement right after acc. E.g. "parallel", "kernels", "host_data", "data", and "exit data" . If the directive applies to a region of code, the directive and the region together are a construct. Clauses are the other parts of a statement that come after the directive (like "collapse(n)" or "async").

abishekg7 · 2025-01-16T16:55:25Z

@abishekg7, I think everything is looking good except this sentence in the commit message

The parallel clauses now specify a default(present)

This seems like an incomplete thought, like there should be more after "...sent)". You could drop the "a" or replace the sentence with something like:

The parallel constructs use default(present) clauses.

With something about "to enforce data management" potentially being added too.

Aside: As I understand OpenACC terminology, directives refer to the part of the statement right after acc. E.g. "parallel", "kernels", "host_data", "data", and "exit data" . If the directive applies to a region of code, the directive and the region together are a construct. Clauses are the other parts of a statement that come after the directive (like "collapse(n)" or "async").

Thanks @gdicker1! I've been using clauses, directives and constructs interchangeably and it's good to know. Will fix this.

This commit enables the GPU execution of the atm_compute_vert_imp_coefs subroutine using OpenACC directives for the data movements and loops. A new timer, 'atm_compute_vert_imp_coefs [ACC_data_xfer]', has been added for host- GPU data transfers in this subroutine. This port follows these considerations: - Separating the computation of cofrz into a separate parallel clause for correctness. - Variables b_tri and c_tri are declared private in lieu of extending the storage to two dimensions, like a_tri. - The data transfers for the invariant fields have been moved to mpas_atm_dynamics_init and finalize. - Explicitly adding gang, worker and vector level parallelism to parallel constructs in atm_compute_vert_imp_coefs_work. - The parallel constructs use default(present) clauses to avoid implicit data movements by the compiler.

abishekg7 · 2025-01-16T17:33:22Z

Fixed the default(present) part of the commit message.

gdicker1

Looks good to me now!

(And I like what you added much more than my suggestion.)

mgduda

Looks good to me!

mgduda requested review from mgduda and gdicker1 November 1, 2024 20:41

mgduda added Atmosphere OpenACC Work related to OpenACC acceleration of code labels Nov 1, 2024

abishekg7 force-pushed the atmosphere/port_atm_compute_vert_imp_coefs branch from bd7bbdd to da32982 Compare November 6, 2024 22:51

gdicker1 reviewed Jan 2, 2025

View reviewed changes

gdicker1 suggested changes Jan 2, 2025

View reviewed changes

gdicker1 mentioned this pull request Jan 2, 2025

Enable GPU exection of atm_rk_integration_setup via OpenACC #1223

Merged

abishekg7 commented Jan 15, 2025

View reviewed changes

src/core_atmosphere/dynamics/mpas_atm_time_integration.F Outdated Show resolved Hide resolved

abishekg7 force-pushed the atmosphere/port_atm_compute_vert_imp_coefs branch from 76a5681 to 63b1e58 Compare January 15, 2025 16:06

mgduda reviewed Jan 16, 2025

View reviewed changes

src/core_atmosphere/dynamics/mpas_atm_time_integration.F Outdated Show resolved Hide resolved

mgduda self-requested a review January 16, 2025 01:16

abishekg7 force-pushed the atmosphere/port_atm_compute_vert_imp_coefs branch from eeecb13 to d2b6428 Compare January 16, 2025 16:27

abishekg7 force-pushed the atmosphere/port_atm_compute_vert_imp_coefs branch from d2b6428 to ba661fc Compare January 16, 2025 17:32

gdicker1 approved these changes Jan 16, 2025

View reviewed changes

mgduda approved these changes Jan 17, 2025

View reviewed changes

mgduda merged commit 589bbe6 into MPAS-Dev:develop Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable GPU execution of atm_compute_vert_imp_coefs via OpenACC #1241

Enable GPU execution of atm_compute_vert_imp_coefs via OpenACC #1241

abishekg7 commented Oct 23, 2024 •

edited

Loading

mgduda commented Nov 1, 2024

abishekg7 commented Nov 6, 2024

gdicker1 Jan 2, 2025

abishekg7 Jan 9, 2025

gdicker1 Jan 2, 2025

abishekg7 Jan 9, 2025

abishekg7 Jan 15, 2025

gdicker1 Jan 2, 2025

abishekg7 Jan 9, 2025

gdicker1 Jan 2, 2025

abishekg7 Jan 9, 2025

gdicker1 left a comment

mgduda commented Jan 16, 2025

abishekg7 commented Jan 16, 2025

gdicker1 commented Jan 16, 2025 •

edited

Loading

abishekg7 commented Jan 16, 2025

abishekg7 commented Jan 16, 2025

gdicker1 left a comment •

edited

Loading

mgduda left a comment

Enable GPU execution of atm_compute_vert_imp_coefs via OpenACC #1241

Enable GPU execution of atm_compute_vert_imp_coefs via OpenACC #1241

Conversation

abishekg7 commented Oct 23, 2024 • edited Loading

mgduda commented Nov 1, 2024

abishekg7 commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdicker1 left a comment

Choose a reason for hiding this comment

mgduda commented Jan 16, 2025

abishekg7 commented Jan 16, 2025

gdicker1 commented Jan 16, 2025 • edited Loading

abishekg7 commented Jan 16, 2025

abishekg7 commented Jan 16, 2025

gdicker1 left a comment • edited Loading

Choose a reason for hiding this comment

mgduda left a comment

Choose a reason for hiding this comment

abishekg7 commented Oct 23, 2024 •

edited

Loading

gdicker1 commented Jan 16, 2025 •

edited

Loading

gdicker1 left a comment •

edited

Loading