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.

Sign up for GitHub

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

omp_locks: Avoid extern global variable #3798

Merged
merged 1 commit into from
Mar 14, 2024
+11 −7
Merged

omp_locks: Avoid extern global variable #3798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions Src/Base/AMReX_BaseFab.H
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3360,9 +3360,7 @@ BaseFab<T>::lockAdd (const BaseFab<T>& src, const Box& srcbox, const Box& destbo
while (planes_left > 0) {
AMREX_ASSERT(mm < nplanes);
auto const m = mm + plo;
int ilock = m % OpenMP::nlocks;
if (ilock < 0) { ilock += OpenMP::nlocks; }
auto* lock = &(OpenMP::omp_locks[ilock]);
auto* lock = OpenMP::get_lock(m);
if (omp_test_lock(lock))
{
auto lo = dlo;
Expand Down
3 changes: 1 addition & 2 deletions Src/Base/AMReX_OpenMP.H
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ namespace amrex::OpenMP {
void Initialize ();
void Finalize ();

static constexpr int nlocks = 128;
extern AMREX_EXPORT omp_lock_t omp_locks[nlocks];
omp_lock_t* get_lock (int ilock);
Copy link
Member

@ax3l ax3l Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since it complains about this, maybe an export here helps:

Suggested change
omp_lock_t* get_lock (int ilock);
AMREX_EXPORT omp_lock_t* get_lock (int ilock);

Copy link
Member

@ax3l ax3l Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Testing:

Copy link
Member

@ax3l ax3l Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Still the same issue on Windows:

[48/49] C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\app.dir\src\main.cpp.obj  /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  lib\libimpactx.NOMPI.OMP.DP.OPMD.lib  lib\buildInfopyImpactX.lib  lib\ablastr_3d.lib  %PREFIX%\Library\lib\amrex_3d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink impactx.NOMPI.OMP.DP.OPMD.exe D:/bld/impactx_1710432645630/work/build/bin/impactx""
FAILED: bin/impactx.NOMPI.OMP.DP.OPMD.exe 
C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_exe --intdir=CMakeFiles\app.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\app.dir\src\main.cpp.obj  /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console  lib\libimpactx.NOMPI.OMP.DP.OPMD.lib  lib\buildInfopyImpactX.lib  lib\ablastr_3d.lib  %PREFIX%\Library\lib\amrex_3d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E create_symlink impactx.NOMPI.OMP.DP.OPMD.exe D:/bld/impactx_1710432645630/work/build/bin/impactx""
LINK: command "%BUILD_PREFIX%\Library\bin\lld-link.exe CMakeFiles\app.dir\src\main.cpp.obj /out:bin\impactx.NOMPI.OMP.DP.OPMD.exe /implib:lib\impactx.NOMPI.OMP.DP.OPMD.lib /pdb:bin\impactx.NOMPI.OMP.DP.OPMD.pdb /version:0.0 /machine:x64 /INCREMENTAL:NO /subsystem:console lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=1" failed (exit code 1) with the following output:
lld-link: error: undefined symbol: __declspec(dllimport) struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int)
>>> referenced by libimpactx.NOMPI.OMP.DP.OPMD.lib(ChargeDeposition.cpp.obj):(public: class amrex::BaseFab<double> & __cdecl amrex::BaseFab<double>::lockAdd<1>(class amrex::BaseFab<double> const &, class amrex::Box const &, class amrex::Box const &, int, int, int))

[49/49] C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\pyImpactX.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj  /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO  -flto  lib\libimpactx.NOMPI.OMP.DP.OPMD.lib  lib\buildInfopyImpactX.lib  lib\ablastr_3d.lib  %PREFIX%\Library\lib\amrex_3d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\libs\python310.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E copy_directory D:/bld/impactx_1710432645630/work/src/python/impactx D:/bld/impactx_1710432645630/work/build/lib/site-packages/impactx""
FAILED: lib/site-packages/impactx/impactx_pybind.cp310-win_amd64.pyd 
C:\Windows\system32\cmd.exe /C "cd . && %BUILD_PREFIX%\Library\bin\cmake.exe -E vs_link_dll --intdir=CMakeFiles\pyImpactX.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- %BUILD_PREFIX%\Library\bin\lld-link.exe  CMakeFiles\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj  /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO  -flto  lib\libimpactx.NOMPI.OMP.DP.OPMD.lib  lib\buildInfopyImpactX.lib  lib\ablastr_3d.lib  %PREFIX%\Library\lib\amrex_3d.lib  %PREFIX%\Library\lib\libomp.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\Library\lib\openPMD.lib  %PREFIX%\Library\lib\adios2_cxx11.lib  %PREFIX%\libs\python310.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && C:\Windows\system32\cmd.exe /C "cd /D %SRC_DIR%\build && %BUILD_PREFIX%\Library\bin\cmake.exe -E copy_directory D:/bld/impactx_1710432645630/work/src/python/impactx D:/bld/impactx_1710432645630/work/build/lib/site-packages/impactx""
LINK: command "%BUILD_PREFIX%\Library\bin\lld-link.exe CMakeFiles\pyImpactX.dir\src\python\pyImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\distribution.cpp.obj CMakeFiles\pyImpactX.dir\src\python\elements.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactX.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ImpactXParticleContainer.cpp.obj CMakeFiles\pyImpactX.dir\src\python\ReferenceParticle.cpp.obj CMakeFiles\pyImpactX.dir\src\python\transformation.cpp.obj /out:lib\site-packages\impactx\impactx_pybind.cp310-win_amd64.pyd /implib:lib\site-packages\impactx\impactx_pybind.lib /pdb:lib\site-packages\impactx\impactx_pybind.pdb /dll /version:0.0 /machine:x64 /INCREMENTAL:NO -flto lib\libimpactx.NOMPI.OMP.DP.OPMD.lib lib\buildInfopyImpactX.lib lib\ablastr_3d.lib %PREFIX%\Library\lib\amrex_3d.lib %PREFIX%\Library\lib\libomp.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\Library\lib\openPMD.lib %PREFIX%\Library\lib\adios2_cxx11.lib %PREFIX%\libs\python310.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=2" failed (exit code 1) with the following output:
lld-link: warning: ignoring unknown argument '-flto'
lld-link: error: undefined symbol: __declspec(dllimport) struct omp_lock_t * __cdecl amrex::OpenMP::get_lock(int)

>>> referenced by libimpactx.NOMPI.OMP.DP.OPMD.lib(ChargeDeposition.cpp.obj):(public: class amrex::BaseFab<double> & __cdecl amrex::BaseFab<double>::lockAdd<1>(class amrex::BaseFab<double> const &, class amrex::Box const &, class amrex::Box const &, int, int, int))

ninja: build stopped: subcommand failed.

Copy link
Member

Choose a reason for hiding this comment

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

Could this be missing includes? #3802

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see any missing includes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's add a test that uses lockAdd to amrex/Tests/ to see if conda-forge Windows build work for amrex.

Copy link
Member

@ax3l ax3l Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't see any missing includes.

I wondered how <omp.h> and AMReX_OpenMP.H come into downstream files such as ImpactX ChargeDeposition.cpp and AMReX_BaseFab.H for BaseFab::lockAdd. AMReX_MultiFab.H does not seem to include it directly, must be some transitive path. Anyway, #3802 does some cleaning for directly uses classes for future robustness.

}

#else // AMREX_USE_OMP
Expand Down
11 changes: 9 additions & 2 deletions Src/Base/AMReX_OpenMP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ namespace amrex
#ifdef AMREX_USE_OMP
namespace amrex::OpenMP
{
omp_lock_t omp_locks[nlocks];

namespace {
constexpr int nlocks = 128;
omp_lock_t omp_locks[nlocks];
unsigned int initialized = 0;
}

Expand Down Expand Up @@ -204,5 +204,12 @@ namespace amrex::OpenMP
}
}

omp_lock_t* get_lock (int ilock)
{
ilock = ilock % nlocks;
if (ilock < 0) { ilock += nlocks; }
return omp_locks + ilock;
}

} // namespace amrex::OpenMP
#endif // AMREX_USE_OMP
Loading