[PATCH] D127803: Generate the capture for field when the field is used in openmp region with implicit default in the member function.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added inline comments.


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1873-1877
+    if (auto *PrivateCopy =
+            isOpenMPFDCaptureDecl(Field, Base.get(), IsArrow, OpLoc, &SS,
+                                  /*TemplateKWLoc=*/SourceLocation(), Field,
+                                  FoundDecl, /*HadMultipleCandidates=*/false,
+                                  MemberNameInfo, MemberType, VK, OK))
----------------
ABataev wrote:
> Why do we need this function? Implicit private rule should apply (if should) only to this-Юашдув kind of expressions, if something like parent.field expression is used, parent should be private, not private.field. Or I'm missing something?
You are right for "parent.a" only privatize "parent".

But if 'a' is a field access in a member function then 'a' is privatized, not "this".  It is same with firstprivate(a).  But for the explicit firstprivate(a), the capture is build in ActOnOpenMPFirstprivateClause, so it can be mapped to reference in the omp region.  For Implicit, I need to build capture for the first reference in the omp region with defalut(first|private) is used.  And used that to generate firstprivate clause at end when the call to ActOnOpenMPFirstprivateClause when generating implicit clause.

Is this reasonable?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:201
+    using ImplicitFDCapTy = std::pair<ImpilitFDLevelTy, VarDecl *>;
+    // List of captuer fields
+    llvm::SmallVector<ImplicitFDCapTy, 8> ImplicitDefaultFirstprivateFDs;
----------------
ABataev wrote:
> ///
Thanks Changed


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2410
         D,
-        [](OpenMPClauseKind C, bool AppliedToPointee) {
+        [](OpenMPClauseKind C, bool AppliedToPointee, ...) {
           return isOpenMPPrivate(C) && !AppliedToPointee;
----------------
ABataev wrote:
> Better to add an actual param type here but without Param name
Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127803