[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new module and a check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri marked 2 inline comments as not done.
lebedev.ri added inline comments.


================
Comment at: docs/clang-tidy/checks/openmp-use-default-none.rst:12
+being implicitly determined, and thus forces developer to be explicit about the
+desired data scoping for each variable.
+
----------------
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > JonasToth wrote:
> > > > 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.
> > > 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?
> > 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?
> 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.
> 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.


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:53
+// 'parallel' directive can have 'default' clause, and said clause is specified,
+// with 'none' kind, all good.
+void t5(const int a) {
----------------
JonasToth wrote:
> Please add basic test-cases for the other constructs that are allowed to have the `default` specified.
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)?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57113/new/

https://reviews.llvm.org/D57113