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

Hybrid Parallel AD (Part 1/?) #1214

Merged
merged 66 commits into from
Apr 7, 2021
Merged

Hybrid Parallel AD (Part 1/?) #1214

merged 66 commits into from
Apr 7, 2021

Conversation

jblueh
Copy link
Contributor

@jblueh jblueh commented Mar 1, 2021

Proposed Changes

The goal of this PR is to establish AD support for the OpenMP features of SU2.

AD support in SU2 is provided by CoDiPack, which is coupled with MeDiPack for the differentiation of MPI. OpenMP support is added by coupling it in addition with OpDiLib so that all in all, hybrid parallel AD is achieved.

This involves at least the following steps.

  1. Incorporate OpDiLib into the AD workflow.
  2. Establish thread-safety and parallelization of the discrete adjoint code.
  3. Testing of hybrid parallel adjoints.
  4. Performance optimizations.

Related Work

OpenMP features introduced in #789, #824, #830, #834, #843, #861, #895, #975, #1178, possibly others

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.

@jblueh
Copy link
Contributor Author

jblueh commented Mar 1, 2021

The commits so far address parts of OpDiLib's incorporation into the AD workflow and sort out some additional issues along the lines of the disc_adj_fsi/Airfoild2d testcase. They provide a testing environment that is stable with respect to OpDiLib and can already be used for figuring out some of the more SU2 specific issues. However, they require that SU2 is built with a compiler with OMPT support, for example an up-to-date version of clang++, hence the failing CI builds.

@pcarruscag
Copy link
Member

It looks like the builds are failing because of swig (python wrapper) and a missing definition of size_t.
What does OMPT stand for? OpenMP threads? (Google was not helpful)

@jblueh
Copy link
Contributor Author

jblueh commented Mar 1, 2021

True that, I built it without the python wrapper locally. This build command worked for me.

./meson.py omptestenv --buildtype=debug --warnlevel=2 -Denable-autodiff=true -Denable-directdiff=true -Dwith-omp=true

The "T" in OMPT stands for "tools", see Chapter 4 of the OpenMP 5.1 specification. We also explain it with a focus on AD in our preprint.

@pcarruscag
Copy link
Member

I see, then it should be possible to detect the version of the standard supported by the compiler and only enable the feature in that case. We do that for simd directives for compatibility (hopefully) with the MS compilers.

@su2code su2code deleted a comment from lgtm-com bot Mar 28, 2021
@su2code su2code deleted a comment from lgtm-com bot Mar 28, 2021
@su2code su2code deleted a comment from jblueh Mar 28, 2021
@su2code su2code deleted a comment from lgtm-com bot Mar 28, 2021
@su2code su2code deleted a comment from lgtm-com bot Apr 6, 2021
@su2code su2code deleted a comment from lgtm-com bot Apr 6, 2021
@su2code su2code deleted a comment from lgtm-com bot Apr 6, 2021
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

I'll merge this soon if there are no objections.

Comment on lines -228 to -243
/*--- Note, passiveDoubleBuffer and doubleBuffer point to the same address.
* This is the reason why we have to do the following copy/reordering in two steps. ---*/
/*--- Reorder the data in the buffer. ---*/

/*--- Step 1: Extract the underlying double value --- */
vector<passivedouble> tmpBuffer(nPoint_Recv[size]);

if (!std::is_same<su2double, passivedouble>::value){
for (int jj = 0; jj < VARS_PER_POINT*nPoint_Recv[size]; jj++){
const passivedouble tmpVal = SU2_TYPE::GetValue(doubleBuffer[jj]);
passiveDoubleBuffer[jj] = tmpVal;
/*--- For some AD datatypes a call of the destructor is
* necessary to properly delete the AD type ---*/
doubleBuffer[jj].~su2double();
}
}

/*--- Step 2: Reorder the data in the buffer --- */
Copy link
Member

Choose a reason for hiding this comment

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

These last changes were to fix issues when the RealReverseIndex is used.
We used to have a char buffer that was used for both passive and active doubles, which required these explicit calls to the destructor, but I guess that did not work so well with mpi and all.
Now there is only passivedouble, which means that once something goes into the data sorter AD information is lost.
This would matter if some computation was performed with time averaged values, which does not seem to be the case at the moment...

Comment on lines 318 to 324
void SetUnsorted_Data(unsigned long iPoint, unsigned short iField, su2double data){
connSend[Index[iPoint] + iField] = data;
connSend[Index[iPoint] + iField] = SU2_TYPE::GetValue(data);
}

su2double GetUnsorted_Data(unsigned long iPoint, unsigned short iField) const {
passivedouble GetUnsorted_Data(unsigned long iPoint, unsigned short iField) const {
return connSend[Index[iPoint] + iField];
}
Copy link
Member

Choose a reason for hiding this comment

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

This is were the active-passive conversion takes place.

CFEMDataSorter* volumeSorter; //!< Pointer to the volume sorter instance
const CFEMDataSorter* volumeSorter; //!< Pointer to the volume sorter instance
Copy link
Member

Choose a reason for hiding this comment

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

also the usual cleanups

@su2code su2code deleted a comment from lgtm-com bot Apr 6, 2021
@lgtm-com
Copy link

lgtm-com bot commented Apr 6, 2021

This pull request fixes 8 alerts when merging 45cc9a5 into e823be7 - view on LGTM.com

fixed alerts:

  • 5 for Comparison of narrow type with wide type in loop condition
  • 3 for Resource not released in destructor

@pcarruscag pcarruscag merged commit 3b20982 into develop Apr 7, 2021
@pcarruscag pcarruscag deleted the hybrid_parallel_ad branch April 7, 2021 12:57
Comment on lines -446 to -448
if (solver[iZone][iInst][MESH_0][ADJFEA_SOL]) {
solver[iZone][iInst][MESH_0][ADJFEA_SOL]->SetRecording(geometry[iZone][iInst][MESH_0], config[iZone]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I already thought I dreamed of removing this code bit when I scrolled through #1257 and it was gone (because it was already deleted here) ... that conditional evaluated to true btw and it does not crash for non-FEA cases . Just c++ things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants