[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:7834
+            llvm::APInt Size = CAT->getSize();
+            SizeV = llvm::ConstantInt::get(CGF.SizeTy, Size);
+          } else if (VAT) {
----------------
cchen wrote:
> ABataev wrote:
> > 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?
> My comment was not accurate, I've updated it. What I want to convey is that we can only have `CAT, VAT, or pointer` here, since analysis in Sema has a restriction for it. (SemaOpenMP line 16623)
It does not relate to the comments thread but I got it. Anyway, try to investigate why the compiler crashes if you try to cr4eate a constant ща ]СПАюШте64Ен] directly.


================
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:
> > 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?
> Yes, I'm fine either passing index or passing address, though I'm curious why you're recommending passing address.
It is going to simplify the codegen. Currently, to get the offset, you need to dig through all the elements of the array section. If, instead, you use the pointers, you would not need to do this and you can rely on something like `CGF.EmitArraySectionLValue()`. At least, I hope so.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7910
+        const Expr *CountExpr = nullptr;
+        if (OASE)
+          CountExpr = OASE->getLength();
----------------
The check is not required, you already checked that the expression must be array section only.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972