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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
