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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Also, what about compatibility with declare mapper? Can you add tests for it?



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7809-7813
+    MapValuesArrayTy CurOffsets{llvm::ConstantInt::get(CGF.CGM.Int64Ty, 0)};
+    MapValuesArrayTy CurCounts{llvm::ConstantInt::get(CGF.CGM.Int64Ty, 1)};
+    MapValuesArrayTy CurStrides;
+    SmallVector<llvm::Value *, 4> DimSizes{
+        llvm::ConstantInt::get(CGF.CGM.Int64Ty, 1)};
----------------
DO not use braced initializer list https://llvm.org/docs/CodingStandards.html#id26


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7839-7847
+        if (CAT) {
+          ElementType = CAT->getElementType().getTypePtr();
+        } else if (VAT) {
+          ElementType = VAT->getElementType().getTypePtr();
+        } else {
+          assert(&Component == &*Components.begin() &&
+                 "Only expect pointer (non CAT or VAT) when this is the "
----------------
No need for braces here per coding standard


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7876
+
+    // Skip the dummy dimension since we have already have its information.
+    auto DI = DimSizes.begin() + 1;
----------------
have already have


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7892
+      const Expr *AssocExpr = Component.getAssociatedExpression();
+      AssocExpr->dump();
+      const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
----------------
debug code


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7893
+      AssocExpr->dump();
+      const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
+      const auto *AE = dyn_cast<ArraySubscriptExpr>(AssocExpr);
----------------
Move this after the first `if` statement


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7894-7896
+      const auto *AE = dyn_cast<ArraySubscriptExpr>(AssocExpr);
+
+      if (AE) {
----------------
Better to make it something like:
```
if (const auto *AE = dyn_cast<ArraySubscriptExpr>(AssocExpr))
```


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7944-7945
+                  : llvm::ConstantInt::get(CGF.Int64Ty, /*V=*/1);
+          Count = CGF.Builder.CreateUDiv(CGF.Builder.CreateNUWSub(*DI, Offset),
+                                         Stride);
+        }
----------------
If no stride at all, why need to divide?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7967
+      DimProd = CGF.Builder.CreateNUWMul(DimProd, *(DI - 1));
+      CurStrides.push_back(CGF.Builder.CreateNUWMul(DimProd, Stride));
+      if (DI != DimSizes.end())
----------------
Same, if no stride at all, no need to mul.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16913-16919
+        if (OASE) {
+          if (!OASE->getLength())
+            SemaRef.Diag(ELoc, diag::err_array_section_does_not_specify_length)
+                << ERange;
+          else
+            break;
+        }
----------------
Better to transform it to something like:
```
if (!OASE || OASE->getLength())
  break;
SemaRef.Diag(...)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84192