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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth 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.
+
----------------
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.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57113