[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: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);
----------------
cchen wrote:
> ABataev wrote:
> > The code for `SizeV` must be under the control of the next `if`:
> > ```
> > if (DimSizes.size() < Components.size() - 1) {
> >  ....
> > }
> > ```
> I don't think I understand this one. Why do you remove SizeV in the if condition?
This is for `SizeV`. You don't use it if `DimSizes.size() < Components.size() - 1` is `false`, looks like memory leak.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7834
+            llvm::APInt Size = CAT->getSize();
+            SizeV = llvm::ConstantInt::get(CGF.SizeTy, Size);
+          } else if (VAT) {
----------------
cchen wrote:
> ABataev wrote:
> > Create directly as of `CGF.Int64Ty` type.
> Doing this I'll get assertion error in this exact line if on a 32-bits target.
Hmm, why, can you investigate?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7872-7873
+          } else {
+            Offset = CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(OffsetExpr),
+                                               CGF.Int64Ty,
+                                               /*isSigned=*/false);
----------------
cchen wrote:
> ABataev wrote:
> > Do you really to pass real offsets here? Can we use pointers instead?
> Do you mean I should set the type of Offset to Expr*?
Currently, you're passing offsets to the runtime. Can we pass pointers instead? I mean, for `a[b]` you pass `b` to the runtime, can we pass `&a[b]` instead?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7334
+          OverlappedElements = llvm::None,
+      MapDimArrayTy *const Dims = nullptr) const {
     // The following summarizes what has to be generated for each map and the
----------------
Can we encapsulate `Dims` into `StructNonContiguousInfo`?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7837-7838
+                Context.getTypeSizeInChars(ElementType).getQuantity();
+          } else if (VAT) {
+            ElementType = VAT->getElementType().getTypePtr();
+            ElementTypeSize =
----------------
If only `CAT` or `VAT` is allowed, then transform this if into:
```
else {
  assert(VAT&& ...);
```


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7875-7878
+        const auto *OASE = dyn_cast<OMPArraySectionExpr>(AssocExpr);
+
+        if (!OASE)
+          continue;
----------------
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:7917
+        if (DI != DimSizes.end()) {
+          CurStride = CGF.Builder.CreateNUWMul(CurStrides.back(), *DI++);
+          CurStrides.push_back(CurStride);
----------------
Avoid expressions with some side effects, like `*DI++`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972