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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1125
+    DVar.CKind = OMPC_firstprivate;
+    DVar.PrivateCopy = nullptr;
+    return DVar;
----------------
Need to add `DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;` 


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3211-3213
+          VD->getStorageClass() == SC_Static &&
+          (CanonicalVD->getDeclContext()->isNamespace() ||
+           !VD->isLocalVarDeclOrParm())) {
----------------
I think just `!VD->hasLocalStorage()` should be enough here. Shall it work for static locals too or just for globals?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:12616-12621
   if (Kind == OMP_DEFAULT_none)
     DSAStack->setDefaultDSANone(KindKwLoc);
   else if (Kind == OMP_DEFAULT_shared)
     DSAStack->setDefaultDSAShared(KindKwLoc);
+  else if (Kind == OMP_DEFAULT_firstprivate)
+    DSAStack->setDefaultDSAFirstPrivate(KindKwLoc);
----------------
Better to turn it to `switch`


================
Comment at: clang/test/OpenMP/parallel_master_codegen.cpp:145
+
+// CK31:       define internal {{.*}}void [[OMP_OUTLINED]](i32* noalias [[GTID:%.+]], i32* noalias [[BTID:%.+]], i32* dereferenceable(4) [[A_VAL]])
+// CK31:       [[GTID_ADDR:%.+]] = alloca i32*
----------------
Some extra work is required. The variable should not be captured by reference, must be captured by value. Also, a test with calling constructors/destructors is required.


================
Comment at: clang/test/OpenMP/parallel_master_codegen.cpp:149
+// CK31:       [[A_ADDR:%.+]] = alloca i32*
+// CK31:       [[A_VAL]]1 = alloca i32, align 4
+// CK31:       store i32* [[GTID]], i32** [[GTID_ADDR]]
----------------
This check may not work in some cases, better to rework it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591