[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:7327-7330
+      bool IsNonContiguous = false,
+      MapNonContiguousArrayTy *const Offsets = nullptr,
+      MapNonContiguousArrayTy *const Counts = nullptr,
+      MapNonContiguousArrayTy *const Strides = nullptr) const {
----------------
I would prefer to pack these 4 params into a single parameter (a struct). Also, can we put `Dims` parameter into the list of the optional parameters?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7805
+      // should be [10, 10] and the first stride is 4 btyes.
+      for (const auto &Component : Components) {
+        const Expr *AssocExpr = Component.getAssociatedExpression();
----------------
Expand `auto` here to a real type


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7807
+        const Expr *AssocExpr = Component.getAssociatedExpression();
+        const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
+        if (OASE) {
----------------
Can we have anything else except for array section here? If not, use just `cast`. If yes, use `continue` to simplify complexity:
```
if (!OASE)
  continue;
...
```


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7821
+                  Context.getTypeSizeInChars(ElementType).getQuantity();
+            } else if (VAT) {
+              ElementType = VAT->getElementType().getTypePtr();
----------------
What if the base is a pointer, not an array?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7831-7838
+          llvm::Value *SizeV = nullptr;
+          if (CAT) {
+            llvm::APInt Size = CAT->getSize();
+            SizeV = llvm::ConstantInt::get(CGF.SizeTy, Size);
+          } else if (VAT) {
+            const Expr *Size = VAT->getSizeExpr();
+            SizeV = CGF.EmitScalarExpr(Size);
----------------
The code for `SizeV` must be under the control of the next `if`:
```
if (DimSizes.size() < Components.size() - 1) {
 ....
}
```


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7834
+            llvm::APInt Size = CAT->getSize();
+            SizeV = llvm::ConstantInt::get(CGF.SizeTy, Size);
+          } else if (VAT) {
----------------
Create directly as of `CGF.Int64Ty` type.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7859
+      // declaration in target update to/from clause.
+      for (const auto &Component : Components) {
+        const Expr *AssocExpr = Component.getAssociatedExpression();
----------------
Expand `auto` here to a real type


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7861-7864
+        const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
+
+        if (OASE) {
+          // Offset
----------------
Can we have anything else except for array section here? If not, use just `cast`. If yes, use `continue` to simplify complexity:
```
if (!OASE)
  continue;
...
```


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7872-7873
+          } else {
+            Offset = CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(OffsetExpr),
+                                               CGF.Int64Ty,
+                                               /*isSigned=*/false);
----------------
Do you really to pass real offsets here? Can we use pointers instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972