[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Do you have the tests for static locals and static data members with `default(firstprivate)`?



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2787
+  if (getLangOpts().OpenMP < 51 && Kind == OMPC_default &&
+      static_cast<DefaultKind>(Val.getValue().Type) ==
+          OMP_DEFAULT_firstprivate) {
----------------
No need for cast here. 


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2789
+          OMP_DEFAULT_firstprivate) {
+    // if (getLangOpts().OpenMP < 51 && Val.getValue().Type == 2) {
+    Diag(Val.getValue().LOpen, diag::err_omp_invalid_dsa)
----------------
Remove this commented line of code.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2793
+        << getOpenMPClauseName(OMPC_default) << "5.1";
+  }
   return Actions.ActOnOpenMPSimpleClause(
----------------
There should be `return nullptr;` after the `Diag()`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3434-3435
+      if (Stack->getDefaultDSA() == DSA_firstprivate &&
+          VD->getStorageDuration() == SD_Static &&
+          CanonicalVD->getDeclContext()->isFileContext() &&
           !Stack->isLoopControlVariable(VD).first) {
----------------
Hmm, maybe move this check to `getDSA()`? If you do it, you can just modify the check on line 3322 
```
      if (DVar.CKind == OMPC_unknown && (Stack->getDefaultDSA() == DSA_none || (Stack->getDefaultDSA() == DSA_firstprivate && !Stack->isLoopControlVariable(VD).first)) &&
          isImplicitOrExplicitTaskingRegion(DKind) &&
          VarsWithInheritedDSA.count(VD) == 0) {
        VarsWithInheritedDSA[VD] = E;
        return;
      }
```


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3441-3446
+      // Create implicit firstprivate variables as necessary under
+      // default(firstprivate).
+      if (Stack->getDefaultDSA() == DSA_firstprivate) {
         ImplicitFirstprivate.push_back(E);
         return;
       }
----------------
Hmm, not sure that this check is needed here, the next if statement should handle it already, no? `DVar.CKind` is set to `OMPC_firstprivate` and the next check must work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591