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

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 4 inline comments as done.
cchen added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7821
+                  Context.getTypeSizeInChars(ElementType).getQuantity();
+            } else if (VAT) {
+              ElementType = VAT->getElementType().getTypePtr();
----------------
ABataev wrote:
> What if the base is a pointer, not an array?
The `if (ElementType)` condition only push back stride when base is not pointer. I'm now allowing one dimension size to be unknown (pointer as base) and sema has analysis to check if more than one indirection as base. My last codegen test case is for testing pointer as base.


================
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);
----------------
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?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7834
+            llvm::APInt Size = CAT->getSize();
+            SizeV = llvm::ConstantInt::get(CGF.SizeTy, Size);
+          } else if (VAT) {
----------------
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.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7872-7873
+          } else {
+            Offset = CGF.Builder.CreateIntCast(CGF.EmitScalarExpr(OffsetExpr),
+                                               CGF.Int64Ty,
+                                               /*isSigned=*/false);
----------------
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*?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972