Page MenuHomePhabricator

[clang-tidy] openmp-use-default-none - a new check
ClosedPublic

Authored by lebedev.ri on Jan 23 2019, 11:56 AM.

Details

Summary

Finds OpenMP directives that are allowed to contain default clause,
but either don't specify it, or the clause is specified but with the kind
other than none, and suggests to use default(none) clause.

Using default(none) clause changes the default variable visibility from
being implicitly determined, and thus forces developer to be explicit about the
desired data scoping for each variable.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Jan 25 2019, 5:48 AM
test/clang-tidy/openmp-use-default-none.cpp
27

AFAIK default(private) should exist as well, please add tests for the other kinds, too.

68

why rewrite the default(shared)? I don't exactly understand the reason for not accepting default(shared).

ABataev added inline comments.Jan 25 2019, 6:09 AM
test/clang-tidy/openmp-use-default-none.cpp
27

No, only default(none|shared) exists

lebedev.ri marked 4 inline comments as done.Jan 25 2019, 6:12 AM
lebedev.ri added inline comments.
clang-tidy/openmp/UseDefaultNoneCheck.cpp
52

It's tricky logic.
There are three possible scenarios here:

  • default(none) is specified, all good.
  • default(shared) is specified, shared!=none, diagnose.
  • No default clause specified. Only a single default clause may be specified on a parallel, task, taskloop or teams directive. (2.19.4.1 default Clause, last line of paragraph). Now, there are two possible cases:
    • We are in a directive (e.g. parallel) that is allowed to have the default clause, but does not have it. Naturally, do diagnose this.
    • We are in a directive (e.g. for) that is not allowed to have the default clause. Naturally, don't diagnose this.

So this is correct.

98

Could work, will take a look, thanks for a hint!

132

Hm, i can drop it, but in previous reviews i have seen this assert being requested to be *added*.

136

Good point about "allowed", i'll see how this can be trimmed..

test/clang-tidy/openmp-use-default-none.cpp
27

In Fortran - yes, but not in C / C++:
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf
Page 282 (302 in pdf):

2.19.4.1 default Clause
...
C / C++
The syntax of the default clause is as follows:
default(shared | none)
68

This check enforces default(none). If the default of shared is specified,
that is still not none, therefore it is incorrect and should be diagnosed.

Why not shared but none? I tried to cover that in the doc,
basically it helps prevent issues by forcing one to be explicit about the "visibilities"
of the variables.

I suppose it the default can be configurable,
i'm just not aware of any reason why one would specify shared.

lebedev.ri marked 4 inline comments as done and an inline comment as not done.Jan 25 2019, 6:14 AM

@JonasToth thanks for taking a look BTW!

JonasToth added inline comments.Jan 25 2019, 6:33 AM
docs/clang-tidy/checks/openmp-use-default-none.rst
13

If I understand correctly the issue is more about implicitly shared variables that lead to data-races and bad access patterns, is that correct?
If so it might clarify the reason for this check, if added directly in the first motivational sentence.

test/clang-tidy/openmp-use-default-none.cpp
68

I have the honor to work on a originally fortran77 scientific code (which is getting modernized step by step) but there it is very common to have 100 variables with maximum length of 4 characters for the variable names.
One would expect the same quality for similar C-Code when OMP beeing used, so there might be the practical reason that 20 lines of declarations what variable is what are unreadable (because the rest is a big desaster already). But in that case one will not use this check..

JonasToth added inline comments.Jan 25 2019, 6:58 AM
clang-tidy/openmp/UseDefaultNoneCheck.cpp
114

Is that the case? Will the pragmas be ignored if non-omp?

You could reorder that sentence Don't register the check if OpenMP is not enabled as the directives are not considered in the AST. or so.

132

You can leave it, but please add an message to the assertion.

clang-tidy/openmp/UseDefaultNoneCheck.h
19

contain _a_ `default` clause

20

double back-tick in dont, please use the normal straight tick instead

21

to use _the_

docs/clang-tidy/checks/openmp-use-default-none.rst
7

not sure, but i think all the commas in this sentence can be dropped. The comma in front of or for sure.

12

forces _developers_

13

is it scoping or rather sharing? The scope is C++-terrain or at least used in C++-Speak. Maybe there is a unambiguous word instead?

29

Very long line. You can move the warning to the next line and wrap it to match the 80 columns limit.

test/clang-tidy/openmp-use-default-none.cpp
54

Please add basic test-cases for the other constructs that are allowed to have the default specified.

ABataev added inline comments.Jan 25 2019, 7:02 AM
clang-tidy/openmp/UseDefaultNoneCheck.cpp
114

If OpenMP is disabled, the pragmas are just completely ignored.

lebedev.ri marked 2 inline comments as done.Jan 25 2019, 7:09 AM
lebedev.ri added inline comments.
docs/clang-tidy/checks/openmp-use-default-none.rst
13

I'm not quite sure how to formulate the reasoning to be honest.
Let me try to show some reasonably-complete example:
https://godbolt.org/z/mMQhbr

We have 4 compilers: clang trunk + openmp 3.1, clang trunk + openmp 4, gcc 8, gcc trunk

We have 5 code samples, all operating with a const variable: (the same code, just different omp clauses)

  • If no default clause is specified, every compiler is fine with the code.
  • If default(shared) clause is specified, every compiler is still fine with the code. On this example, no clause and default(shared) clause are equivalent. I can't/won't claim whether or not that is always the case, as i'm mostly arguing re default(none).
  • If default(none) shared(num) is specified, all is also fine. That is equivalent to just the default(none) for OpenMP 3.1
  • If default(none) firstprivate(num) is specified, all still fine fine.
  • If only `default(none) is specified, things start to get wonky. The general idea is that before OpenMP 4.0 such const variables were auto-determined as shared, and now they won't. So one will need to add either shared(num) or firstprivate(num). Except the older gcc will diagnose shared(num) :)

Roughly the same is true when the variable is not const: https://godbolt.org/z/wuouI_

Thus, default(none) forced one to be explicit about what shall be done with the variable,
should it be shared, or firstprivate.

^ Not whether this rambling makes sense?

test/clang-tidy/openmp-use-default-none.cpp
68

Oh yeah, i'm quite sure that is common for non-fortran code too.

JonasToth added inline comments.Jan 25 2019, 7:22 AM
docs/clang-tidy/checks/openmp-use-default-none.rst
13

Yes, so its about being explicit if state is shared or not. Given that its hard to reason about parallel programs being explicit helps reading the code. I think that could be condensed in a short motivation section.

lebedev.ri marked 2 inline comments as not done.Jan 25 2019, 7:35 AM
lebedev.ri added inline comments.
docs/clang-tidy/checks/openmp-use-default-none.rst
13

Yes, so its about being explicit if state is shared or not. Given that its hard to reason about parallel programs being explicit helps reading the code. I think that could be condensed in a short motivation section.

Yep. Will try to reword.

is it scoping or rather sharing? The scope is C++-terrain or at least used in C++-Speak. Maybe there is a unambiguous word instead?

E.g. clang messages say variable 'num' must have explicitly specified data sharing attributes
So "data sharing" i suppose.

test/clang-tidy/openmp-use-default-none.cpp
54

For the base directives (so 3x4 new tests) i assume,
not all the possible combinations of directives that are allowed to contain default (a *LOT* more tests)?

lebedev.ri marked 28 inline comments as done.

Addressed review notes;
found a way to use the OMPClause matcher in isAllowedToContainClause(),
as opposed to passing the OpenMPClauseKind.

clang-tidy/openmp/UseDefaultNoneCheck.cpp
50–51

Solved, this is so ugly xD

98

Not quite sure re Kind suffix, but i guess this is better.

114

Reworded a little, hopefully better.

clang-tidy/openmp/UseDefaultNoneCheck.h
20

Hah, that is mass-replace for you.

docs/clang-tidy/checks/openmp-use-default-none.rst
13

Reworded, hopefully better?

Adjust a few comments.

New module is still not mentioned in docs/clang-tidy/index.rst.

Mention new module in docs/clang-tidy/index.rst.

JonasToth accepted this revision.Jan 27 2019, 5:07 AM

LGTM with the few language nits.
The new matchers look better and are more matcher style, gj :)

clang-tidy/openmp/UseDefaultNoneCheck.cpp
127

typo, entries

docs/clang-tidy/checks/openmp-use-default-none.rst
11

Using the

15

making errors easier to spot. the more is not necessary.

This revision is now accepted and ready to land.Jan 27 2019, 5:07 AM

LGTM with the few language nits.
The new matchers look better and are more matcher style, gj :)

Thank you for the review!
I'm not quite sure who is qualified to review the base
D57112 ASTTypeTraits patch though, @klimek or @sbenza it looks like?

NFC.

  • Rebased
  • Switched to OpenMPClauseKind ASTNodeKind::getOMPClauseKind(). This is better anyway, since it avoids string comparisons, and is a simple switch over an enum.
lebedev.ri updated this revision to Diff 184692.Feb 1 2019, 1:37 AM
lebedev.ri retitled this revision from [clang-tidy] openmp-use-default-none - a new module and a check to [clang-tidy] openmp-use-default-none - a new check.
lebedev.ri edited the summary of this revision. (Show Details)

NFC, rebased, split the new check from the new module into separate differentials.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 1:37 AM
alexfh removed a reviewer: alexfh.Feb 25 2019, 2:50 PM
lebedev.ri edited the summary of this revision. (Show Details)

Rebased, NFC.
Moved matchers into D59453+D57112.

lebedev.ri marked 4 inline comments as done.
lebedev.ri edited the summary of this revision. (Show Details)
lebedev.ri added a reviewer: gribozavr.

Rebased, NFC.

Address some nits.

aaron.ballman added inline comments.Mar 22 2019, 7:17 AM
clang-tidy/openmp/UseDefaultNoneCheck.cpp
48–49

Make the diagnostic ungrammatical.

53

Same with notes.

59–60

Here too.

docs/clang-tidy/checks/openmp-use-default-none.rst
20

This is a *lot* of example text -- are you sure all of it adds value, or can some of it be removed?

lebedev.ri marked 5 inline comments as done.

Make diags less grammatically correct, reduce docs.

docs/clang-tidy/checks/openmp-use-default-none.rst
20

Right, let's just leave one example :)

aaron.ballman accepted this revision.Mar 22 2019, 12:15 PM

LGTM aside from a minor diagnostic wording nit.

clang-tidy/openmp/UseDefaultNoneCheck.cpp
53

drop the "is" to make it match how we usually phrase things.

LGTM aside from a minor diagnostic wording nit.

Thank you for the review!

This revision was automatically updated to reflect the committed changes.