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

[Atmn Patel via Phabricator via llvm-commits]

atmnpatel marked 2 inline comments as done.
atmnpatel added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3211-3213
+          VD->getStorageClass() == SC_Static &&
+          (CanonicalVD->getDeclContext()->isNamespace() ||
+           !VD->isLocalVarDeclOrParm())) {
----------------
ABataev wrote:
> atmnpatel wrote:
> > ABataev wrote:
> > > I think just `!VD->hasLocalStorage()` should be enough here. Shall it work for static locals too or just for globals?
> > Just for globals.
> What about static data members?
The TR doesn't specify that static data members should inherit DSAs / have them explicitly defined.


================
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*
----------------
ABataev wrote:
> atmnpatel wrote:
> > ABataev wrote:
> > > 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.
> > Sorry, I added a test with constructors/destructors with codegen. The variable capture discussion was had above and I'm pretty sure that the conclusion above (correct me if I'm wrong) was that its not ideal, but its fine for now and will be adjusted with the upcoming OMPIRBuilder.
> It shall emit the correct code anyway. With `default(firstprivate)` and explicit `firstprivate` clause the codegen will be different and it may lead to an incompatibility with the existing libraries/object files.
Ohh I see your concern now. Is the conclusion that I should just table this until something new pops up? I couldn't follow up on @fghanim's advice (sorry I've been on and off trying different things for weeks), it's beyond my capabilities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591