[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:3211-3213
+          VD->getStorageClass() == SC_Static &&
+          (CanonicalVD->getDeclContext()->isNamespace() ||
+           !VD->isLocalVarDeclOrParm())) {
----------------
atmnpatel wrote:
> 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.
What about non-static globals? Your current check works only for something like `static int var;`, but not `int var;`.


================
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*
----------------
atmnpatel wrote:
> 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.
You need to modify function `Sema::isOpenMPCapturedByRef`. There is a check for firstprivates in there, you need to add a check that the variable has no explicit data-sharing attributes and the default data sharing is set to `firstprivate`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75591