Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend #1269

abishekg7 · 2025-01-17T21:39:11Z

Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend

gdicker1

Besides a more formal commit message, here's some things I've noticed so far.

gdicker1 · 2025-01-21T19:10:41Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      call mpas_pool_get_config(config, 'config_relax_zone_divdamp_coef', divdamp_coef_ptr)
+
+      divdamp_coef = divdamp_coef_ptr
+      vertexDegree = vertexDegree_ptr


I think it would be good to add a comment here, just a few words about avoiding non-array Fortran pointers for OpenACC.

Thread bump/reminder.

Thanks! Done.

gdicker1 · 2025-01-21T22:48:06Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc parallel default(present)
+      !$acc loop gang worker private(cell1, cell2, vertex1, vertex2, r_dc, r_dv, & 
+      !$acc    iCell, iVertex, invArea, iEdge_div, iEdge_vort, edge_sign, &
+      !$acc    laplacian_filter_coef, divergence1, divergence2, vorticity1, vorticity2)


Here I'd only expect the arrays (divergence1, divergence2, vorticity1, and vorticity2) to be declared private. The rest should automatically be private at the worker level and passed on to their (vector) threads. Are you being explicit here, or do you need to also add the scalar-values to the private clause for correctness?

True. I briefly didn't remember that about scalars when I wasn't getting bit identical results. Will make these changes and try again.

gdicker1 · 2025-01-21T22:48:08Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

      do iEdge = edgeStart, edgeEnd

         if ( (bdyMaskEdge(iEdge) > 1) .and. (bdyMaskEdge(iEdge) <= nRelaxZone) ) then ! relaxation zone

-            laplacian_filter_coef = dcEdge(iEdge)**2 * (real(bdyMaskEdge(iEdge)) - 1.)/   &
+            laplacian_filter_coef = dcEdge(iEdge)*dcEdge(iEdge)* (real(bdyMaskEdge(iEdge)) - 1.)/   &


Pure curiosity about this change: could you explain your motivation a little? Does this have any effect?

I was trying this out because I though it may be one of the obstacles to bit identical results - but probably not. Will remove this change. Although, I would also add that we could just be using the multiplication operator here instead of exponent. But that's for another time.

Agreed, I think this may be better as a multiplication than exponent but that goes against keeping to a "light touch" in this phase.

jim-p-w · 2025-01-21T23:04:20Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc end parallel

+      !$acc parallel default(present)


Can both of these loops be in the same parallel region and run simultaneously? They appear to be orthogonal to each other. That is, neither loop references the array(s) the other loop changes.

Thanks Jim. Will try that out.

Fusing these two parallel regions does work, but I'm wondering if we should perhaps leave this for the optimization stage.

I am not knowledgable enough to weigh in on that. But if it succeeded and BFB tests passed I'd put it in now.
I'm fine with it either way, I'll leave it to you, Dylan, and Michael to decide.

jim-p-w · 2025-01-21T23:12:34Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

      do iCell = cellSolveStart, cellSolveEnd ! threaded over cells

         if ( (bdyMaskCell(iCell) > 1) .and. (bdyMaskCell(iCell) <= nRelaxZone) ) then ! relaxation zone

            laplacian_filter_coef = (real(bdyMaskCell(iCell)) - 1.)/real(nRelaxZone)/(10.*dt*meshScalingRegionalCell(iCell))
            !
+            !$acc loop seq


Given that local scalars are automatically privatized, it seems this loop doesn't have to execute sequentially.

So there's a summation going on in the innermost loop for tend_rt and tend_rho, and ignoring the order of summation, there would still be an incorrect summation if do i=1,nEdgesOnCell(iCell) was parallelized.

Now we could re-write the loop to use the acc reduction(+: tend_rt, tend_rho) clause, but we decided to keep the code changes minimal in this round of porting.

I missed that! Thanks for double checking.

jim-p-w · 2025-01-21T23:21:09Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F


      !  Third (and last), the horizontal filter for ru
-
+      !$acc parallel default(present)
+      !$acc loop gang worker private(cell1, cell2, vertex1, vertex2, r_dc, r_dv, & 


Local scalar vars are automatically privatized. Additionally, there's a performance hit when explicitly declaring them private, as it causes them to be placed in global memory. See this nvidia forum item

Thanks, yeah I'll remove the scalars from private. And did not know this about global memory - thanks!

jim-p-w · 2025-01-21T23:25:44Z

src/core_atmosphere/dynamics/mpas_atm_time_integration.F

+      !$acc parallel default(present)
+      !$acc loop gang worker private(cell1, cell2, vertex1, vertex2, r_dc, r_dv, & 
+      !$acc    iCell, iVertex, invArea, iEdge_div, iEdge_vort, edge_sign, &
+      !$acc    laplacian_filter_coef, divergence1, divergence2, vorticity1, vorticity2)


Why does laplacian_filter_coef have to be private in this loop? It's not declared private in the other parallel loops in this subroutine.

Yes, it can be removed from private too.

gdicker1 · 2025-02-03T18:47:32Z

This looks good to me now. Just waiting for any changes from Michael's review and the final commit(s)

mgduda requested review from mgduda, gdicker1 and jim-p-w January 17, 2025 22:33

mgduda added Atmosphere OpenACC Work related to OpenACC acceleration of code labels Jan 17, 2025

gdicker1 suggested changes Jan 21, 2025

View reviewed changes

jim-p-w reviewed Jan 21, 2025

View reviewed changes

abishekg7 added 3 commits January 31, 2025 15:33

First working version

8ae0a8f

Removing scalars from private clause and reverting exponent operation

cef6ed8

adding comment

5c7d198

abishekg7 force-pushed the atmosphere/port_atm_bdy_adjust_dynamics_relaxzone_tend branch from 2e993c5 to 5c7d198 Compare January 31, 2025 22:54

jim-p-w approved these changes Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend #1269

Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend #1269

abishekg7 commented Jan 17, 2025

gdicker1 left a comment

gdicker1 Jan 21, 2025

abishekg7 Jan 22, 2025

gdicker1 Jan 31, 2025

abishekg7 Jan 31, 2025

gdicker1 Jan 21, 2025

abishekg7 Jan 22, 2025

gdicker1 Jan 21, 2025

abishekg7 Jan 22, 2025

gdicker1 Jan 22, 2025

jim-p-w Jan 21, 2025

abishekg7 Jan 22, 2025

abishekg7 Jan 31, 2025

jim-p-w Jan 31, 2025

jim-p-w Jan 21, 2025

abishekg7 Jan 22, 2025

jim-p-w Jan 22, 2025

jim-p-w Jan 21, 2025

abishekg7 Jan 22, 2025

jim-p-w Jan 21, 2025

abishekg7 Jan 22, 2025

gdicker1 commented Feb 3, 2025

Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend #1269

Are you sure you want to change the base?

Initial OpenACC port of atm_bdy_adjust_dynamics_relaxzone_tend #1269

Conversation

abishekg7 commented Jan 17, 2025

gdicker1 left a 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

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

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 commented Feb 3, 2025