Use default(none) for OpenMP definitions #3524

SunBlack · 2020-01-06T00:37:20Z

Fix warnings reported by run-clang-tidy -header-filter='.*' -checks='-*,openmp-use-default-none'

Some notes, whch i didn't touched here:

I believe there are currently some bugs in PCL if OpenMP is enabled, due to wrong expectation about usage of private (see here). In general I don't see any good use case for private if values are always uninitialized.
#ifdef _OPENMP is mostly unnecessary, as Any pragma that is not recognized is ignored.
I don't see any reason, why tests are using OpenMP. Test cases should be small enough, that spawning new threads should be more expensive than without them.
for-ranged loops are supported only since OpenMP 5.0, so we have to decide: Use OpenMP or for-ranged loops? (maybe drop OpenMP support in general as only really few parts of PCL are using it?)

SunBlack · 2020-01-06T15:20:45Z

Not sure if we can really apply this as:

Ubuntu 19.04 fails with:

/__w/1/s/common/src/fft/kiss_fft.c:262:13: error: 'fstride' not specified in enclosing 'parallel'
  262 |             kf_work( Fout +k*m, f+ fstride*in_stride*k,fstride*p,in_stride,factors,st);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If I fix it Ubuntu 16.04 fails with:

/__w/1/s/common/src/fft/kiss_fft.c:260:9: error: 'fstride' is predetermined 'shared' for 'shared'
   shared(f, factors, Fout, fstride, in_stride, m, p, st)
         ^

kunaltyagi · 2020-01-08T06:42:54Z

+1 for default(none)
Agree on the private sharing (this behavior was expected), I feel firstprivate is what's intended in most of the uses of private
Regarding _OPENMP, that's sometimes used for getting number of threads, so some use still needs that, but I'm down with less use of all macros (specially redundant ones)

tracking/include/pcl/tracking/impl/pyramidal_klt.hpp

surface/include/pcl/surface/impl/mls.hpp

registration/include/pcl/registration/impl/ia_fpcs.hpp

SunBlack · 2020-01-08T15:50:51Z

I setup now an environment like Azure Ubuntu 19.10 image on my machine to get an successful build. It seems to be really impossible to get this working as:

If my old setup (18.04) is compiling successfully, 19.10 and 16.04 is broken on Azure
If I fix it for 19.10, 18.04 and 16.04 are broken
If I fix it for 16.04, 18.04 and 19.10 are broken.

Reasons seems to be different supported versions of OpenMP:

Ubuntu 16.04: GCC 5.4.0 => OpenMP 4.0
Ubuntu 18.04: GCC 7.4.0 => OpenMP 4.5
Ubuntu 19.10: GCC 9.2.1 => OpenMP 5.0

So I still prefer to remove support of OpenMP, as it looks completely random, when it is used and when not and it prevent us of modernize some code, because of old OpenMP versions.

kunaltyagi · 2020-01-08T16:20:24Z

This link suggests to not put const variables in the shared list since they are by virtue of constness, shared.

This may help in finding common ground between 4.0 and 4.5 at least

Could you paste the errors in all 3 versions? I can only check 2 (my system: GCC 9 and Azure)

SunBlack · 2020-01-08T17:13:24Z

Could you paste the errors in all 3 versions? I can only check 2 (my system: GCC 9 and Azure)

See here ;-)

SunBlack · 2020-01-08T17:43:35Z

This may help in finding common ground between 4.0 and 4.5 at least

I saw now that #pragma omp parallel for is wrong and #pragma omp for is correct, which doesn't have default(none). Well, this hurts^^

Seems shared isn't available, too and I don't see any alias.

kunaltyagi · 2020-01-08T17:53:09Z

Don't be hasty. Please check Pg 2, right column. parallel for is a valid construct

Also, I figured out the solution to your problem. I have 2 solutions working on godbolt

Apparently, parallel creates jobs for the thread_pool and for divides up the task. If there's no parallel enclosing for, then it's just additional overhead for nothing.

SunBlack · 2020-01-09T10:21:40Z

Don't be hasty. Please check Pg 2, right column. parallel for is a valid construct

You are right. Happens if you take a short look 2 minutes before to knock off work.

Also, I figured out the solution to your problem. I have 2 solutions working on godbolt

Well I believe we shouldn't check for GCC-Version, but for _OpenMP value (values are from spec):

OpenMP 4.0: 201307
OpenMP 4.5: 201511
OpenMP 5.0: 201811

Interesting thing: Your online compiler says GCC 9.2 only have OpenMP 4.5 support during the OpenMP website says it should can 5.0.

Interesting thing: OpenMP spec (Chapter 7.2) says

Beginning with OpenMP 4.0, variables with const-qualified type and no mutable member are no longer predetermined shared. Thus, these variables (variable c in the example) need to be explicitly
listed in data-sharing attribute clauses when the default(none) clause is specified

But GCC 5.4 behaves like before OpenMP 4.0 😕 Maybe I should add an output to CMake, which prints supported OpenMP version, so we can check it.

But we really want a lot of #ifdefs?

UnaNancyOwen · 2020-01-09T10:29:13Z

NOTE: Visual C++ only supports OpenMP 2.0. OpenMP 3.0 and later is not supported in Visual C++.
https://docs.microsoft.com/en-us/cpp/build/reference/openmp-enable-openmp-2-0-support?view=vs-2019

kunaltyagi · 2020-01-09T10:31:28Z

But we really want a lot of #ifdefs?

Ideally, no. But like @UnaNancyOwen just said, we might not escape that. I really really look forwards to OpenMP executors to eliminate pre-preprocessor compiler magic.

Regarding the rest of OpenMP version and GCC versions: 🤷‍♂ 🤦‍♂

SunBlack · 2020-01-09T10:33:15Z

Maybe I should add an output to CMake, which prints supported OpenMP version, so we can check it.

Well this doesn't work, no idea why 😖 (I'm using CMake 3.16 and the variables are empty)

  if (${CMAKE_VERSION} VERSION_LESS "3.7")
    message(STATUS "Found OpenMP")
  elseif (${CMAKE_VERSION} VERSION_LESS "3.9")
    message(STATUS "Found OpenMP, version ${OpenMP_CXX_SPEC_DATE}")
  else()
    message(STATUS "Found OpenMP, version ${OpenMP_CXX_VERSION}")
  endif()

kunaltyagi · 2020-01-09T10:36:51Z

Maybe look at the setup output. Mine gives the following information (without modifications):

-- Found OpenMP_C: -fopenmp (found version "4.5") 
-- Found OpenMP_CXX: -fopenmp (found version "4.5") 
-- Found OpenMP: TRUE (found version "4.5")  
-- Found OpenMP

~~The variables might not be setup properly, so you'll need to hunt for them in config files.~~

I'm using CMake 3.16 and the variables are empty

Oh, I think you missed this in CMake documentation. It states:

The module will also try to provide the OpenMP version variables

As in not guaranteed

SunBlack · 2020-01-09T11:02:52Z

Maybe look at the setup output. Mine gives the following information (without modifications):

I see it at Ubuntu 19.10 Azure build, but not on 16.04. Even my machine with CMake 3.16 is silent. And I don't see a message call for it in FindOpenMP.cmake, so no idea where it will be printed.
Btw: But found out, that ${OpenMP_CXX_VERSION} is broken.

Changed find_package call of OpenMP, because I don't believe we need Fortran OpenMP compiler, which will be searched by default.

NOTE: Visual C++ only supports OpenMP 2.0. OpenMP 3.0 or later is not supported in Visual C++.

True, VS2017 reports as supported spec version 200203 😆

SunBlack · 2020-01-09T11:10:15Z

Oh, I think you missed this in CMake documentation. It states:

The module will also try to provide the OpenMP version variables

As in not guaranteed

Well I don't think I missed it, as there is already an issue ;-)

SunBlack · 2020-02-02T21:26:26Z

I hope it compiles now all. I tested it with gcc-8, gcc-9 & clang-8. Because of this stupid thing I need to check for GCC version and not just for the version of OpenMP.

kunaltyagi

Compiles 🚀 Good hunting @SunBlack

features/include/pcl/features/impl/shot_lrf_omp.hpp

keypoints/include/pcl/keypoints/impl/trajkovic_3d.hpp

SunBlack · 2020-03-09T16:57:58Z

@kunaltyagi I suggest to let this PR as it is and to not change behavior, so I'm solving your first first 3 suggestions without applying, because they have nothing to do with clang-tidy. Feel free to create some PRs to adjust them ;-)

CMakeLists.txt

taketwo · 2020-03-11T08:23:09Z

common/src/fft/kiss_fft.c

@@ -255,7 +255,15 @@ void kf_work(
        int k;

        // execute the p different work units in different threads
-#       pragma omp parallel for
+#if defined _OPENMP && _OPENMP <= 201307 || (__GNUC__ >= 6 && __GNUC__ < 9)


This expression is repeated in multiple files and looks extremely cryptic. What does it test, why? I'd suggest we give it a descriptive name (e.g. OPENMP_SUPPORTS_XYZ) and put it in pcl_macros.h.

I added OPENMP_LEGACY_CONST_DATA_SHARING_RULE now (except in kiss_fft.c as this is a C file and can't include pcl_macros.h)

Could you create a todo-issue because one of my branch's pcl_macros addresses this issue? That'll be resolved after a few merges though.

Sorry for late response. Did you already have a PR for your changes related to pcl_macros or an issue where I can attach this information?

Perfect, thank you! I will adjust it, when this is merged.

…-*,openmp-use-default-none'

kunaltyagi

LGTM. CI doesn't show any warnings related to OpenMP.

taketwo

The CMake part and the macro LGTM. However, I don't have enough experience with OpenMP to review the "meat" of this PR, so we'll have to rely on your judgement, @kunaltyagi

SunBlack mentioned this pull request Jan 6, 2020

[Overview] Modernization code with clang-tidy #2731

Open

SunBlack force-pushed the openmp-use-default-none branch 2 times, most recently from 142e83f to c7fbd7a Compare January 6, 2020 13:43

kunaltyagi reviewed Jan 8, 2020

View reviewed changes

tracking/include/pcl/tracking/impl/pyramidal_klt.hpp Show resolved Hide resolved

surface/include/pcl/surface/impl/mls.hpp Show resolved Hide resolved

registration/include/pcl/registration/impl/ia_fpcs.hpp Show resolved Hide resolved

SunBlack force-pushed the openmp-use-default-none branch from 057fcfa to e4cfd09 Compare January 8, 2020 17:41

SunBlack force-pushed the openmp-use-default-none branch from cc609e0 to 144b27c Compare January 9, 2020 11:04

SunBlack force-pushed the openmp-use-default-none branch from 144b27c to 426b839 Compare January 9, 2020 12:53

SunBlack force-pushed the openmp-use-default-none branch 3 times, most recently from 0990fa9 to 8260982 Compare February 2, 2020 21:24

kunaltyagi reviewed Feb 3, 2020

View reviewed changes

features/include/pcl/features/impl/shot_lrf_omp.hpp Outdated Show resolved Hide resolved

keypoints/include/pcl/keypoints/impl/trajkovic_3d.hpp Outdated Show resolved Hide resolved

kunaltyagi added the needs: more work Specify why not closed/merged yet label Feb 21, 2020

SunBlack force-pushed the openmp-use-default-none branch from 8260982 to 04fa716 Compare March 9, 2020 16:52

taketwo reviewed Mar 11, 2020

View reviewed changes

kunaltyagi added changelog: fix Meta-information for changelog generation skill: openmp Skills/areas of expertise needed to tackle the issue labels Mar 11, 2020

SunBlack force-pushed the openmp-use-default-none branch 3 times, most recently from 69511e8 to 839d068 Compare March 11, 2020 16:28

Heiko Thiel added 2 commits March 25, 2020 15:25

Fix warnings reported by run-clang-tidy -header-filter='.*' -checks='…

b3d8dbd

…-*,openmp-use-default-none'

Print OpenMP version during CMake setup

3bc8ca9

SunBlack force-pushed the openmp-use-default-none branch from 839d068 to 3bc8ca9 Compare March 25, 2020 14:30

kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Mar 26, 2020

kunaltyagi approved these changes Mar 26, 2020

View reviewed changes

kunaltyagi requested a review from taketwo March 26, 2020 06:27

taketwo approved these changes Mar 27, 2020

View reviewed changes

kunaltyagi merged commit 522fe4a into PointCloudLibrary:master Mar 28, 2020

SunBlack deleted the openmp-use-default-none branch March 28, 2020 14:43

SunBlack mentioned this pull request Mar 29, 2020

Select OpenMP data sharing mode based on specific GCC versions #3823

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use default(none) for OpenMP definitions #3524

Use default(none) for OpenMP definitions #3524

SunBlack commented Jan 6, 2020

SunBlack commented Jan 6, 2020

kunaltyagi commented Jan 8, 2020 •

edited

Loading

SunBlack commented Jan 8, 2020

kunaltyagi commented Jan 8, 2020

SunBlack commented Jan 8, 2020

SunBlack commented Jan 8, 2020 •

edited

Loading

kunaltyagi commented Jan 8, 2020 •

edited

Loading

SunBlack commented Jan 9, 2020

UnaNancyOwen commented Jan 9, 2020 •

edited

Loading

kunaltyagi commented Jan 9, 2020 •

edited

Loading

SunBlack commented Jan 9, 2020

kunaltyagi commented Jan 9, 2020 •

edited

Loading

SunBlack commented Jan 9, 2020 •

edited

Loading

SunBlack commented Jan 9, 2020

SunBlack commented Feb 2, 2020

kunaltyagi left a comment

SunBlack commented Mar 9, 2020

taketwo Mar 11, 2020

SunBlack Mar 11, 2020

kunaltyagi Mar 11, 2020

SunBlack Mar 25, 2020

kunaltyagi Mar 25, 2020

SunBlack Mar 25, 2020 •

edited

Loading

kunaltyagi left a comment

taketwo left a comment

Use default(none) for OpenMP definitions #3524

Use default(none) for OpenMP definitions #3524

Conversation

SunBlack commented Jan 6, 2020

SunBlack commented Jan 6, 2020

kunaltyagi commented Jan 8, 2020 • edited Loading

SunBlack commented Jan 8, 2020

kunaltyagi commented Jan 8, 2020

SunBlack commented Jan 8, 2020

SunBlack commented Jan 8, 2020 • edited Loading

kunaltyagi commented Jan 8, 2020 • edited Loading

SunBlack commented Jan 9, 2020

UnaNancyOwen commented Jan 9, 2020 • edited Loading

kunaltyagi commented Jan 9, 2020 • edited Loading

SunBlack commented Jan 9, 2020

kunaltyagi commented Jan 9, 2020 • edited Loading

SunBlack commented Jan 9, 2020 • edited Loading

SunBlack commented Jan 9, 2020

SunBlack commented Feb 2, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

SunBlack commented Mar 9, 2020

taketwo Mar 11, 2020

Choose a reason for hiding this comment

SunBlack Mar 11, 2020

Choose a reason for hiding this comment

kunaltyagi Mar 11, 2020

Choose a reason for hiding this comment

SunBlack Mar 25, 2020

Choose a reason for hiding this comment

kunaltyagi Mar 25, 2020

Choose a reason for hiding this comment

SunBlack Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

kunaltyagi commented Jan 8, 2020 •

edited

Loading

SunBlack commented Jan 8, 2020 •

edited

Loading

kunaltyagi commented Jan 8, 2020 •

edited

Loading

UnaNancyOwen commented Jan 9, 2020 •

edited

Loading

kunaltyagi commented Jan 9, 2020 •

edited

Loading

kunaltyagi commented Jan 9, 2020 •

edited

Loading

SunBlack commented Jan 9, 2020 •

edited

Loading

SunBlack Mar 25, 2020 •

edited

Loading