[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 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:
> 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?


================
Comment at: test/clang-tidy/openmp-use-default-none.cpp:67
+  // CHECK-NOTES: :[[@LINE-3]]:9: warning: OpenMP directive 'parallel for' is allowed to contain the 'default' clause, and 'default' clause exists with 'shared' kind. Consider using 'default(none)' clause instead.
+  // CHECK-NOTES: :[[@LINE-4]]:26: note: Existing 'default' clause is specified here.
+}
----------------
JonasToth wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > why rewrite the `default(shared)`? I don't exactly understand the reason for not accepting `default(shared)`.
> > 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`.
> 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..
Oh yeah, i'm quite sure that is common for non-fortran code too.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113