[PATCH] D79972: [OpenMP5.0] map item can be non-contiguous for target update

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Do you have a test for mapping of something like `arr[0][:n]`, where the base is an array subscript and the remaining part is an array section?



================
Comment at: clang/include/clang/AST/OpenMPClause.h:4680-4681
+        : AssociatedExpressionNonContiguousPr(
+              llvm::PointerIntPair<Expr *, 1, bool>(AssociatedExpression,
+                                                    IsNonContiguous)),
           AssociatedDeclaration(
----------------
I think you can initialize `AssociatedExpressionNonContiguousPr` using just `AssociatedExpressionNonContiguousPr(AssociatedExpression, IsNonContiguous)` form, no?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7890
         OpenMPMapClauseKind MapType,
-        ArrayRef<OpenMPMapModifierKind> MapModifiers,
-        bool ReturnDevicePointer, bool IsImplicit)
+        ArrayRef<OpenMPMapModifierKind> MapModifiers, bool ReturnDevicePointer,
+        bool IsImplicit)
----------------
Restore original formatting


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8375-8376
       bool IsFinalArraySection =
-          isFinalArraySectionExpression(I->getAssociatedExpression());
+          isFinalArraySectionExpression(I->getAssociatedExpression()) &&
+          (!IsNonContiguous);
 
----------------
Better to convert it to `!IsNonContiguous && isFinalArraySectionExpression(I->getAssociatedExpression())`.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8396
+      if (OASE)
+        DimSize++;
 
----------------
Use prefix form `++DimSize`.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8453
+                             /*AddIsTargetParamFlag=*/false,
+                             /*IsNonContiguous=*/IsNonContiguous);
           LB = BP;
----------------
No need for parameter name comment here, it is required only if the `true|false` constants are used


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8508
+                             IsCaptureFirstInfo && !RequiresReference,
+                             /*IsNonContiguous=*/IsNonContiguous);
 
----------------
Same, comment not required


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8563-8577
+  /// Generate the base pointers, section pointers, sizes , map type bits,
+  /// dimension size, offset, count, and strides for the provided map type, map
+  /// modifier, and expression components. \a IsFirstComponent should be set to
+  /// true if the provided set of components is the first associated with a
+  /// capture.
+  void generateInfoForTargetDataComponentList(
+      OpenMPMapClauseKind MapType, ArrayRef<OpenMPMapModifierKind> MapModifiers,
----------------
Can we merge the functionality in this new function with the existing ones somehow? It is not the best idea to duplicate functionality using copy-paste if any.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9353-9355
-/// Emit the arrays used to pass the captures and map information to the
-/// offloading runtime library. If there is no map or capture information,
-/// return nullptr by reference.
----------------
Why removed the comment?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9681
+/// return nullptr by reference.
+static void emitTargetDataOffloadingArrays(
+    CodeGenFunction &CGF,
----------------
Same question as before - can we merge this functionality with the existing functions?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:18383-18385
     MVLI.VarComponents.back().push_back(
-        OMPClauseMappableExprCommon::MappableComponent(SimpleRefExpr, D));
+        OMPClauseMappableExprCommon::MappableComponent(SimpleRefExpr, D,
+                                                       false));
----------------
Use `.emplace_back(SimpleRefExpr, D, false);`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12481-12482
     auto *AssociatedDecl = Record.readDeclAs<ValueDecl>();
     Components.push_back(OMPClauseMappableExprCommon::MappableComponent(
-        AssociatedExpr, AssociatedDecl));
+        AssociatedExprPr, AssociatedDecl, /*IsNonContiguous=*/false));
   }
----------------
`.emplace_back(AssociatedExprPr, AssociatedDecl, /*IsNonContiguous=*/false);`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12599-12600
     auto *AssociatedDecl = Record.readDeclAs<ValueDecl>();
     Components.push_back(OMPClauseMappableExprCommon::MappableComponent(
-        AssociatedExpr, AssociatedDecl));
+        AssociatedExprPr, AssociatedDecl, IsNonContiguous));
   }
----------------
Same, use `emplace_back()`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12650
     auto *AssociatedDecl = Record.readDeclAs<ValueDecl>();
     Components.push_back(OMPClauseMappableExprCommon::MappableComponent(
+        AssociatedExprPr, AssociatedDecl, IsNonContiguous));
----------------
Same, use `emplace_back()`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12700
     auto *AssociatedDecl = Record.readDeclAs<ValueDecl>();
     Components.push_back(OMPClauseMappableExprCommon::MappableComponent(
+        AssociatedExprPr, AssociatedDecl, /*IsNonContiguous=*/false));
----------------
Same, use `emplace_back()`


================
Comment at: clang/lib/Serialization/ASTReader.cpp:12744
     Components.push_back(OMPClauseMappableExprCommon::MappableComponent(
-        AssociatedExpr, AssociatedDecl));
+        AssociatedExpr, AssociatedDecl, /*IsNonContiguous*/ false));
   }
----------------
Same, use `emplace_back()`


================
Comment at: clang/test/OpenMP/target_update_to_messages.cpp:147
   }
+
   return 0;
----------------
Delete this extra line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972