[Clang][OpenMP] Capture mapped pointers on target by reference.

clang/lib/CodeGen/CGOpenMPRuntime.cpp

@@ -7275,8 +7275,14 @@ class MappableExprsHandler {
     //     of arguments, hence MEMBER_OF(4)
     //
     // map(p, p[:100])
+    // For "pragma omp target":
+    // &p, &p, sizeof(p), TARGET_PARAM | TO | FROM
+    // &p, &p[0], 100*sizeof(float), PTR_AND_OBJ | TO | FROM (*)
+    // Otherwise:
     // ===> map(p[:100])
     // &p, &p[0], 100*sizeof(float), TARGET_PARAM | PTR_AND_OBJ | TO | FROM
+    // (*) We need to use PTR_AND_OBJ here to ensure that the mapped copies of
+    // p and p[0] get attached.
 
     // Track if the map information being generated is the first for a capture.
     bool IsCaptureFirstInfo = IsFirstComponentList;
@@ -7294,14 +7300,26 @@ class MappableExprsHandler {
     // components.
     bool IsExpressionFirstInfo = true;
     bool FirstPointerInComplexData = false;
+    bool SkipStandalonePtrMapping = false;
     Address BP = Address::invalid();
     const Expr *AssocExpr = I->getAssociatedExpression();
     const auto *AE = dyn_cast<ArraySubscriptExpr>(AssocExpr);
     const auto *OASE = dyn_cast<ArraySectionExpr>(AssocExpr);
     const auto *OAShE = dyn_cast<OMPArrayShapingExpr>(AssocExpr);
 
-    if (AreBothBasePtrAndPteeMapped && std::next(I) == CE)
+    // For map(p, p[0]) on a "target" construct, we need to map "p" by itself
+    // as it has to be passed by-reference as the kernel argument.
+    // For other constructs, we can skip mapping "p" because the PTR_AND_OBJ
+    // mapping for map(p[0]) will take care of mapping p as well.
+    SkipStandalonePtrMapping =
+        AreBothBasePtrAndPteeMapped &&
+        (!isa<const OMPExecutableDirective *>(CurDir) ||
+         !isOpenMPTargetExecutionDirective(
+             cast<const OMPExecutableDirective *>(CurDir)->getDirectiveKind()));
+
+    if (SkipStandalonePtrMapping && std::next(I) == CE)
       return;
+
     if (isa<MemberExpr>(AssocExpr)) {
       // The base is the 'this' pointer. The content of the pointer is going
       // to be the base of the field being mapped.
@@ -7677,7 +7695,7 @@ class MappableExprsHandler {
               getMapTypeBits(MapType, MapModifiers, MotionModifiers, IsImplicit,
                              !IsExpressionFirstInfo || RequiresReference ||
                                  FirstPointerInComplexData || IsMemberReference,
-                             AreBothBasePtrAndPteeMapped ||
+                             SkipStandalonePtrMapping ||
                                  (IsCaptureFirstInfo && !RequiresReference),
                              IsNonContiguous);
 
@@ -8863,8 +8881,56 @@ class MappableExprsHandler {
           CurCaptureVarInfo.append(CurInfoForComponentLists);
         };
 
-    GenerateInfoForComponentLists(DeclComponentLists,
+    // Next, we break-down the lists of components into lists that should be
+    // handled together.
+    //
+    // For now, we handle maps on pointers by themselves, and everything else
+    // together.
+    //
+    // TODO: Do this based on which lists have the same attachable-base-pointer
+    // e.g. The map clauses below, which are present on the same construct,
+    // should be handled grouped together based on their
+    // attachable-base-pointers:
+    //   map-clause                | attachable-base-pointer
+    //   --------------------------+------------------------
+    //   map(p, ps)                | none
+    //   map(p[0])                 | p
+    //   map(p[0]->b, p[0]->c)     | p[0]
+    //   map(ps->d, ps->e, ps->pt) | ps
+    //   map(ps->pt->d, ps->pt->e) | ps->pt
+
+    MapDataArrayTy ListsThatMapPointerVDItself;
+    MapDataArrayTy RemainingLists;
+    bool IsVDPointerType = VD && VD->getType()->isPointerType();
+
+    for (const MapData &L : DeclComponentLists) {
+      if (IsVDPointerType) {
+        OMPClauseMappableExprCommon::MappableExprComponentListRef Components =
+            std::get<0>(L);
+        bool IsMapOfPointerVD =
+            Components.size() == 1 &&
+            Components[0].getAssociatedDeclaration() &&
+            Components[0].getAssociatedDeclaration()->getCanonicalDecl() == VD;
+        if (IsMapOfPointerVD) {
+          ListsThatMapPointerVDItself.push_back(L);
+          continue;
+        }
+      }
+      RemainingLists.push_back(L);
+    }
+
+    // If VD is a pointer, and there are component-lists mapping VD itself,
+    // like: `int *p; ... map(p)`, we handle them first, and
+    // the first one from the should get the TARGET_PARAM flag.
+    GenerateInfoForComponentLists(ListsThatMapPointerVDItself,
                                   /*IsEligibleForTargetParamFlag=*/true);
+    // Then we handle all the other lists together. Note that since we already
+    // added TARGET_PARAM for the pointer case, we shouldn't add it again if
+    // if there are other lists that get handled here, for example, the list
+    // for `map(p[0:10]` in `int *p; ... map(p, p[0:10])`.
+    GenerateInfoForComponentLists(
+        RemainingLists,
+        /*IsEligibleForTargetParamFlag=*/ListsThatMapPointerVDItself.empty());
   }
 
   /// Generate the base pointers, section pointers, sizes, map types, and

clang/lib/Sema/SemaOpenMP.cpp

@@ -2146,6 +2146,7 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
     // | ptr  |      n.a.     |  -  |   x   |       -       |     -    | bycopy|
     // | ptr  |      n.a.     |  x  |   -   |       -       |     -    | null  |
     // | ptr  |      n.a.     |  -  |   -   |       -       |     x    | byref |
+    // | ptr  |      n.a.     |  -  |   -   |       -       |   x, x[] | bycopy|
     // | ptr  |      n.a.     |  -  |   -   |       -       |    x[]   | bycopy|
     // | ptr  |      n.a.     |  -  |   -   |       x       |          | bycopy|
     // | ptr  |      n.a.     |  -  |   -   |       x       |     x    | bycopy|
@@ -2171,18 +2172,22 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
     //  - For pointers mapped by value that have either an implicit map or an
     //    array section, the runtime library may pass the NULL value to the
     //    device instead of the value passed to it by the compiler.
+    //  - If both a pointer an a dereference of it are mapped, then the pointer
+    //    should be passed by reference.
 
     if (Ty->isReferenceType())
       Ty = Ty->castAs<ReferenceType>()->getPointeeType();
 
-    // Locate map clauses and see if the variable being captured is referred to
-    // in any of those clauses. Here we only care about variables, not fields,
-    // because fields are part of aggregates.
+    // Locate map clauses and see if the variable being captured is mapped by
+    // itself, or referred to, in any of those clauses. Here we only care about
+    // variables, not fields, because fields are part of aggregates.
     bool IsVariableAssociatedWithSection = false;
+    bool IsVariableItselfMapped = false;
 
     DSAStack->checkMappableExprComponentListsForDeclAtLevel(
         D, Level,
         [&IsVariableUsedInMapClause, &IsVariableAssociatedWithSection,
+         &IsVariableItselfMapped,
          D](OMPClauseMappableExprCommon::MappableExprComponentListRef
                 MapExprComponents,
             OpenMPClauseKind WhereFoundClauseKind) {
@@ -2198,8 +2203,19 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
 
           assert(EI != EE && "Invalid map expression!");
 
-          if (isa<DeclRefExpr>(EI->getAssociatedExpression()))
-            IsVariableUsedInMapClause |= EI->getAssociatedDeclaration() == D;
+          if (isa<DeclRefExpr>(EI->getAssociatedExpression()) &&
+              EI->getAssociatedDeclaration() == D) {
+            IsVariableUsedInMapClause = true;
+
+            // If the component list has only one element, it's for mapping the
+            // variable itself, like map(p). This takes precedence in
+            // determining how it's captured, so we don't need to look further
+            // for any other maps that use the variable (like map(p[0]) etc.)
+            if (MapExprComponents.size() == 1) {
+              IsVariableItselfMapped = true;
+              return true;
+            }
+          }
 
           ++EI;
           if (EI == EE)
@@ -2213,8 +2229,10 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
               isa<MemberExpr>(EI->getAssociatedExpression()) ||
               isa<OMPArrayShapingExpr>(Last->getAssociatedExpression())) {
             IsVariableAssociatedWithSection = true;
-            // There is nothing more we need to know about this variable.
-            return true;
+            // We've found a case like map(p[0]) or map(p->a) or map(*p),
+            // so we are done with this particular map, but we need to keep
+            // looking in case we find a map(p).
+            return false;
           }
 
           // Keep looking for more map info.
@@ -2223,8 +2241,23 @@ bool SemaOpenMP::isOpenMPCapturedByRef(const ValueDecl *D, unsigned Level,
 
     if (IsVariableUsedInMapClause) {
       // If variable is identified in a map clause it is always captured by
-      // reference except if it is a pointer that is dereferenced somehow.
-      IsByRef = !(Ty->isPointerType() && IsVariableAssociatedWithSection);
+      // reference except if it is a pointer that is dereferenced somehow, but
+      // not itself mapped.
+      //
+      // OpenMP 6.0, 7.1.1: Data sharing attribute rules, variables referenced
+      // in a construct::
+      // If a list item in a has_device_addr clause or in a map clause on the
+      // target construct has a base pointer, and the base pointer is a scalar
+      // variable *that is not a list item in a map clause on the construct*,
+      // the base pointer is firstprivate.
+      //
+      // OpenMP 4.5, 2.15.1.1: Data-sharing Attribute Rules for Variables
+      // Referenced in a Construct:
+      // If an array section is a list item in a map clause on the target
+      // construct and the array section is derived from a variable for which
+      // the type is pointer then that variable is firstprivate.
+      IsByRef = IsVariableItselfMapped ||
+                !(Ty->isPointerType() && IsVariableAssociatedWithSection);
     } else {
       // By default, all the data that has a scalar type is mapped by copy
       // (except for reduction variables).