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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2053
+          D, [](OpenMPClauseKind K) -> bool { return K != OMPC_unknown; },
+          Level, /*NotLastprivate=*/true) &&
+      !DSAStack->isLoopControlVariable(D, Level).first) {
----------------
`/*NotLastprivate=*/true` is not needed here. Also, could you move it under control of the if statement above?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2185-2186
     if (DVarPrivate.CKind != OMPC_unknown ||
-        (VD && DSAStack->getDefaultDSA() == DSA_none))
+        (VD && (DSAStack->getDefaultDSA() == DSA_none ||
+                DSAStack->getDefaultDSA() == DSA_firstprivate)))
       return VD ? VD : cast<VarDecl>(DVarPrivate.PrivateCopy->getDecl());
----------------
Why do you need this check here? `getTopDSA` will return `OMPC_firstprivate` and one of the previous checks should be fired.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3337
+      if (Stack->getDefaultDSA() == DSA_firstprivate &&
+          VD->getStorageClass() == SC_Static &&
+          (CanonicalVD->getDeclContext()->isNamespace() ||
----------------
1. You need to check that the variable is not a loop control variable here.
2. Seems to me you misinterpret the standard. According to the standard `An object whose identifier is declared without the storage-class specifier _Thread_local, and either with external or internal linkage or with the storage-class specifier static, has static storage duration.`. It means that non-static globals also must be affected since they have external linkage. So, you need to use `getStorageDuration()` function here and check that it returns `SD_Static`.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3338-3339
+          VD->getStorageClass() == SC_Static &&
+          (CanonicalVD->getDeclContext()->isNamespace() ||
+           !VD->isLocalVarDeclOrParm())) {
+        VarsWithInheritedDSA[VD] = E;
----------------
I think just `CanonicalVD->getDeclContext()->isFileContext()` must be enough here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3344-3349
+      // Create implicit firstprivate variables as necessary under
+      // default(firstprivate).
+      if (Stack->getDefaultDSA() == DSA_firstprivate) {
+        ImplicitFirstprivate.push_back(E);
+        return;
+      }
----------------
This code must be somewhere later, otherwise, you're skipping a lot of checks for the target-based directives. This must be checked and processed when we request implicit data sharing attributes with `Stack->getImplicitDSA(VD, /*FromParent=*/false);`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12745
   }
+
   return Res;
----------------
Remove this empty line.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12783-12787
+  if (getLangOpts().OpenMP < 51 && Kind == OMP_DEFAULT_firstprivate) {
+    Diag(StartLoc, diag::err_omp_invalid_dsa)
+        << getOpenMPClauseName(OMPC_firstprivate)
+        << getOpenMPClauseName(OMPC_default) << "5.1";
+  }
----------------
Usually, such checks are implemented in ParseOpenMP.cpp rather than in Sema.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591