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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7046-7049
+    /// Signal that the runtime library should use args as an array of
+    /// descriptor_dim pointers and use args_size as dims. Used when we have
+    /// non-contiguous list items in target update directive
+    OMP_MAP_DESCRIPTOR = 0x800,
----------------
I'm not sure about the value of this flag. If I do recall it correctly, this value might be used for something different by XL compiler, for example. Maybe use some other value, maybe high bits? It is a kind of service flag, not data mapping attribute, so better to move it to high bits (the bit before OMP_MAP_MEMBER_OF maybe?).


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7808-7809
+
+      for (auto CI = Components.begin(), CE = Components.end(); CI != CE;
+           ++CI) {
+        const Expr *AssocExpr = CI->getAssociatedExpression();
----------------
Use range-based loop, if possible.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7811-7813
+        const auto *AE = dyn_cast<ArraySubscriptExpr>(AssocExpr);
+        const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
+        if (AE || OASE) {
----------------
Do you really need to analyze array subscript expressions here? I though that we should analyze only array sections, no?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7870
+      // declaration in target update to/from clause.
+      for (; I != CE; ++I) {
+        const Expr *AssocExpr = I->getAssociatedExpression();
----------------
Same, try to use range-based loop, if possible.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7875
+
+        if (OASE || AE) {
+          // Offset
----------------
Same question about array subscript expressions.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8231-8243
+        if (L.Components.back().isNonContiguous()) {
+          generateInfoForComponentList(
+              L.MapType, L.MapModifiers, L.Components, CurBasePointers,
+              CurPointers, CurSizes, CurTypes, CurDims, PartialStruct,
+              IsFirstComponentList, L.IsImplicit,
+              /*OverlappedElements=*/llvm::None,
+              /*IsNonContiguous=*/true, &CurOffsets, &CurCounts, &CurStrides);
----------------
Just `generateInfoForComponentList(
              L.MapType, L.MapModifiers, L.Components, CurBasePointers,
              CurPointers, CurSizes, CurTypes, CurDims, PartialStruct,
              IsFirstComponentList, L.IsImplicit,
              /*OverlappedElements=*/llvm::None,
              L.Components.back().isNonContiguous(), &CurOffsets, &CurCounts, &CurStrides);`


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8783-8786
+    CGOpenMPRuntime::TargetDataInfo &Info, bool IsNonContiguous = false,
+    MappableExprsHandler::MapNonContiguousArrayTy *Offsets = nullptr,
+    MappableExprsHandler::MapNonContiguousArrayTy *Counts = nullptr,
+    MappableExprsHandler::MapNonContiguousArrayTy *Strides = nullptr) {
----------------
Can we encapsulate these new data into `CGOpenMPRuntime::TargetDataInfo`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972