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

[Ready] gcc-9 build fixes #1970

Merged
merged 1 commit into from Aug 10, 2019

Conversation

@lozhnikov
Copy link
Contributor

commented Aug 6, 2019

gcc 9.1.1 throws errors like

src/mlpack/methods/det/dt_utils_impl.hpp:189:3: error: ‘folds’ not specified in enclosing ‘parallel’
src/mlpack/methods/det/dt_utils_impl.hpp:192:33: error: ‘testSize’ not specified in enclosing ‘parallel’
src/mlpack/methods/det/dt_utils_impl.hpp:194:61: error: ‘cvData’ not specified in enclosing ‘parallel’

These variables are used inside omp parallel for default(none) clause and they are not marked as shared. Looks like the previous versions of gcc didn't throw any errors since the variables are declared as const.

@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Looks like it's quite an issue. The static analyzer tells:

src/mlpack/methods/det/dt_utils_impl.hpp:189:61: error: ‘folds’ is predetermined ‘shared’ for ‘shared’
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Awesome, thanks for looking into this. I think it is the same thing as reported in #1928. I hadn't managed to reproduce it (don't have gcc 9 installed, though I guess it would be easy to set up...). What happens if we just drop the default(none)?

If that doesn't work, maybe (unfortunately) some #ifs to check the OpenMP version (or compiler version?) might be needed.

@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Dropping default(none) works fine. However, I think checking the OpenMP version is a more delicate way than dropping `default(none)'.

@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

It's not a bug. The reason is described in detail here https://www.gnu.org/software/gcc/gcc-9/porting_to.html.

The OpenMP 3.1 specifications (https://www.openmp.org/specifications/) state the following rule:

Data-sharing Attribute Rules for Variables Referenced in a Construct

  • Variables with const-qualified type having no mutable member are shared.

The OpenMP 4.0 (and later) specifications don't have this rule.

@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

How about the following fix:

  #if _OPENMP >= 201307
    #pragma omp parallel for default(none) \
        shared(prunedSequence, regularizationConstants, folds, testSize, \
               cvData, minLeafSize, maxLeafSize, useVolumeReg)
  #else
    #pragma omp parallel for default(none) \
        shared(prunedSequence, regularizationConstants)
  #endif

?

@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

No, looks like it doesn't work since gcc 7.3.0 provides the same OpenMP version (namely, 4.5) as gcc 9.1 (see http://ci.mlpack.org/job/pull-requests-mlpack-static-code-analysis/2430/console). Hence the previous versions of gcc won't work with the fix.

Looks like the previous versions of gcc didn't fully support it.

In this case I think it's better to remove the default(none) than write various compiler workarounds.

@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Agreed, if we can get it working without default(none) I think that is better than various compiler workarounds. The _OPENMP idea was really nice, so it's quite unfortunate that it's a compiler behavior change. :(

@lozhnikov lozhnikov force-pushed the lozhnikov:gcc-9-build-fixes branch from ae53391 to 56b9d7b Aug 7, 2019
@lozhnikov lozhnikov force-pushed the lozhnikov:gcc-9-build-fixes branch from 56b9d7b to 51f8565 Aug 8, 2019
@lozhnikov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

The PR is ready.

@lozhnikov lozhnikov changed the title gcc-9 build fixes [Ready] gcc-9 build fixes Aug 9, 2019
@zoq
zoq approved these changes Aug 9, 2019
Copy link
Member

left a comment

Looks good to me.

Copy link
Member

left a comment

Awesome, nice to have this fixed. Thanks for working it out. :)

@rcurtin rcurtin merged commit 51f8565 into mlpack:master Aug 10, 2019
6 checks passed
6 checks passed
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Thanks again; I made some changes to HISTORY.md to point out this fix in 565617b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.