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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to