Hybrid parallel (OpenMP) support for incompressible solvers #1178

pcarruscag · 2021-01-26T23:06:03Z

Proposed Changes

Remove some more duplication, fix some issues, and make the incompressible solvers compatible with OpenMP.

Related Work

Fix #1175

PR Checklist

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
I have added a test case that demonstrates my contribution, if necessary.
I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

…incomp

vdweide · 2021-01-27T07:28:58Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

+        StrainMag_Max = max(StrainMag_Max, strainMax);
+        Omega_Max = max(Omega_Max, omegaMax);
+      }
+


I suppose you do the reduction over the threads yourself because of efficiency, when the if statement is false, but wouldn't it be more readable to have a reduction clause in the OMP for loop?

Thank you for having a look at the code Edwin, the issues with these kind of reductions were:

The microsoft compilers do not support max/min reductions;

For the code to compile with AD (forward) we would need to define custom reduction operators, which might not be very readable;

Earlier discussions about OpenMP+AD (reverse) suggested that avoiding reduction clauses would make it easier to develop a thread-safe AD tool.

For performance the reduction clause would be more efficient, as the compiler can make min/max atomic, however I do not know of a nice simple way to implement atomic min/max.

vdweide · 2021-01-27T07:31:03Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

      SU2_OMP_BARRIER
-      SU2_OMP_MASTER
-      {
+      SU2_OMP_MASTER {


For efficiency reasons then the Allreduce could be done in a single call with two arguments, as for both Omega and the Strain you compute the maximum.

…incomp

vdweide · 2021-01-29T17:05:32Z

SU2_CFD/include/solvers/CIncEulerSolver.hpp

@@ -38,7 +38,7 @@
 */
 class CIncEulerSolver : public CFVMFlowSolverBase<CIncEulerVariable, INCOMPRESSIBLE> {
 protected:
-  CFluidModel *FluidModel = nullptr;    /*!< \brief fluid model used in the solver */
+  vector<CFluidModel*> FluidModel;   /*!< \brief fluid model used in the solver. */


Don't you want to put the fluid model in the base class? CEulerSolver has exactly the same variable.

It is a bit different for the NEMO solver, it also has a "FluidModel" but of a different type.

vdweide

The few questions I had have been answered, so this one looks good to me.

TobiKattmann

Thanks a lot for adding omp support to the inc solver 💐 and moving some reg test accordingly
And thanks for moving some functions to the FVMFlowSolverBase 👍
Much appreciated

TobiKattmann · 2021-02-01T08:37:25Z

SU2_CFD/include/solvers/CEulerSolver.hpp

@@ -37,6 +37,8 @@
 */
 class CEulerSolver : public CFVMFlowSolverBase<CEulerVariable, COMPRESSIBLE> {
 protected:
+  using BaseClass = CFVMFlowSolverBase<CEulerVariable, COMPRESSIBLE>;


You need this to call an implementation of the base class later like BaseClass::SetInitialCondition in this very CEulerSolver. But SetInitialCondition is also public in the FVMBase class so it should be callable anyway because we inherit that via class CEulerSolver : public CFVMFlowSolverBase ... where am I wrong?

If you call SetInitialCondition in SetInitialCondition you get an infinite loop, I just wanted to make more expressive I was calling the implementation of the parent first.

TobiKattmann · 2021-02-01T08:40:18Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

+  inline virtual void Viscous_Residual(unsigned long iEdge, CGeometry *geometry, CSolver **solver_container,
+                                       CNumerics *numerics, CConfig *config) { }
+  void Viscous_Residual_impl(unsigned long iEdge, CGeometry *geometry, CSolver **solver_container,
+                             CNumerics *numerics, CConfig *config);
+  using CSolver::Viscous_Residual; /*--- Silence warning ---*/


The idea behind this to be able to 'fake' override Viscous_Residual for viscous flows with just calling Viscous_Residual_impl because for inviscid flow the empty implementation from here is taken. This trick has to be used because FVMBase -> Euler -> NS is the inheritance and Viscous_Residual itself cannot be overloaded because Euler is the middleman?
And what happens to the call in CIntegration to Viscous_Residual for NS flow. Is there a doubled contribution then because it is called there and in the centered/upwind residual?

Well, in Cintegration the empty impl of CSolver is used and in the centered/upwind residual the correct Viscous_Residual impl with the Viscous_Residual_impl init ...
Seems that i have to go back and read a couple of pages in 'Inheritance for complete morons'. fml

Yep that's all correct.

Except the self flagellation part, no need for that xD

TobiKattmann · 2021-02-01T08:41:08Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

+   * \note The convective residual methods include a call to this for each edge,
+   *       this allows convective and viscous loops to be "fused".


Is there a speed-up due to this? Or is this just for simplification?

Fast code goes brrrrrrrrrrrrrrrrrrrr

TobiKattmann · 2021-02-01T08:50:47Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.inl

+template <class V, ENUM_REGIME R>
+void CFVMFlowSolverBase<V, R>::LoadRestart(CGeometry **geometry, CSolver ***solver,
+                                           CConfig *config, int iter, bool update_geo) {
+  LoadRestart_impl(geometry, solver, config, iter, update_geo);
+}


Can you explain why having the LoadRestart_impl is beneficial? The LoadRestart moved from the Euler and IncEuler to FVMFlowSolverBase so that is good but why the additional wrapper function

TobiKattmann · 2021-02-01T08:56:32Z

SU2_CFD/include/solvers/CIncEulerSolver.hpp

@@ -149,7 +142,7 @@ class CIncEulerSolver : public CFVMFlowSolverBase<CIncEulerVariable, INCOMPRESSI
   * \brief Compute the pressure at the infinity.
   * \return Value of the pressure at the infinity.
   */
-  inline CFluidModel* GetFluidModel(void) const final { return FluidModel;}
+  inline CFluidModel* GetFluidModel(void) const final { return FluidModel[omp_get_thread_num()]; }


Wait, I guess (real) shared-memory is not viable for the FluidModel as it is filled and computes stuff on the fly so each thread gets its own?

TobiKattmann · 2021-02-01T09:20:32Z

SU2_CFD/src/solvers/CEulerSolver.cpp

-      }
-    }
-  }
+  BaseClass::SetInitialCondition(geometry, solver_container, config, TimeIter);


this is the BaseClass call I mean above

TobiKattmann · 2021-02-01T09:28:21Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

@@ -1057,14 +983,21 @@ void CIncEulerSolver::SetTime_Step(CGeometry *geometry, CSolver **solver_contain
 void CIncEulerSolver::Centered_Residual(CGeometry *geometry, CSolver **solver_container, CNumerics **numerics_container,
                                     CConfig *config, unsigned short iMesh, unsigned short iRKStep) {

-  CNumerics* numerics = numerics_container[CONV_TERM];
+  CNumerics* numerics = numerics_container[CONV_TERM + omp_get_thread_num()*MAX_TERMS];


Here you need multiple numerics instances because in the OMP loop they all individually fill the container and evaluate their residual, right?

TobiKattmann · 2021-02-01T09:29:43Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

 void CIncEulerSolver::Centered_Residual(CGeometry *geometry, CSolver **solver_container, CNumerics **numerics_container,
                                     CConfig *config, unsigned short iMesh, unsigned short iRKStep) {



Here it would be prob nice to reflect in the name that it contains the viscous residual.

Yeah... but then I would need to change all the solvers, the vectorized residual loop is called EdgeFluxResidual to be more inclusive...

pcarruscag · 2021-02-01T10:50:41Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

-  delete [] Restart_Data; Restart_Data = nullptr;
+  LoadRestart_impl(geometry, solver, config, val_iter, val_update_geo, Solution, nVar_Restart);



@TobiKattmann to satisfy the quirkiness of the incompressible solver that needs a default value for temperature when there is no energy equation

pcarruscag added 9 commits January 24, 2021 13:26

Merge branch 'stress_penalty' into hybrid_parallel_incomp

9c958b7

BC's, fluid model, restart

203369d

Vorticity/StrainMag and UndivLaplacian

f6bc631

fluxes and sources

d858f70

BetaInc2

bbfd9c0

add incompressible hybrid regressions

fc4cfd1

Merge remote-tracking branch 'upstream/develop' into hybrid_parallel_…

cc2946e

…incomp

Merge branch 'stress_penalty' into hybrid_parallel_incomp

ce04458

fix #1175

5bad047

pr-triage bot added the PR: unreviewed label Jan 26, 2021

pcarruscag added the changelog:feature label Jan 26, 2021

pcarruscag linked an issue Jan 26, 2021 that may be closed by this pull request

Incorrect aero coeffs for compressible moving grid simulations #1175

Closed

vdweide reviewed Jan 27, 2021

View reviewed changes

pcarruscag and others added 8 commits January 27, 2021 15:51

Merge branch 'develop' into hybrid_parallel_incomp

dec8abf

Merge remote-tracking branch 'upstream/develop' into hybrid_parallel_…

396e8d5

…incomp

fix leak set incomp hybrid regression residuals

7bd274a

fix the Roe kappa thing and some compiler warnings

6118dff

fix directdiff compilation with OpenMP

30184d3

one LoadRestart to rule them all

726f4e7

simplify SetInitialCondition

afe4928

even cleaner

a7e6695

vdweide reviewed Jan 29, 2021

View reviewed changes

vdweide approved these changes Jan 29, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 29, 2021

pcarruscag merged commit be378f2 into develop Jan 30, 2021

pcarruscag deleted the hybrid_parallel_incomp branch January 30, 2021 14:12

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Jan 30, 2021

pcarruscag mentioned this pull request Jan 30, 2021

Support for MPI communicators which are provided externally #1180

Merged

5 tasks

TobiKattmann reviewed Feb 1, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: merged labels Feb 1, 2021

pcarruscag commented Feb 1, 2021

View reviewed changes

jblueh mentioned this pull request Mar 1, 2021

Hybrid Parallel AD (Part 1/?) #1214

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hybrid parallel (OpenMP) support for incompressible solvers #1178

Hybrid parallel (OpenMP) support for incompressible solvers #1178

pcarruscag commented Jan 26, 2021

vdweide Jan 27, 2021

pcarruscag Jan 27, 2021

vdweide Jan 27, 2021

vdweide Jan 29, 2021

pcarruscag Jan 29, 2021

vdweide left a comment

TobiKattmann left a comment

TobiKattmann Feb 1, 2021

pcarruscag Feb 1, 2021

TobiKattmann Feb 1, 2021

TobiKattmann Feb 1, 2021

pcarruscag Feb 1, 2021

pcarruscag Feb 1, 2021

TobiKattmann Feb 1, 2021

pcarruscag Feb 1, 2021

TobiKattmann Feb 1, 2021

TobiKattmann Feb 1, 2021

TobiKattmann Feb 1, 2021

TobiKattmann Feb 1, 2021

TobiKattmann Feb 1, 2021

pcarruscag Feb 1, 2021

pcarruscag Feb 1, 2021

		* \note The convective residual methods include a call to this for each edge,
		* this allows convective and viscous loops to be "fused".

		void CIncEulerSolver::Centered_Residual(CGeometry geometry, CSolver solver_container, CNumerics *numerics_container,
		CConfig *config, unsigned short iMesh, unsigned short iRKStep) {

		delete [] Restart_Data; Restart_Data = nullptr;
		LoadRestart_impl(geometry, solver, config, val_iter, val_update_geo, Solution, nVar_Restart);

Hybrid parallel (OpenMP) support for incompressible solvers #1178

Hybrid parallel (OpenMP) support for incompressible solvers #1178

Conversation

pcarruscag commented Jan 26, 2021

Proposed Changes

Related Work

PR Checklist

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

vdweide left a comment

Choose a reason for hiding this comment

TobiKattmann 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