[PATCH] D16749: [OpenMP] Map clause codegeneration.

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Alexey,

Thanks for the review!


================
Comment at: lib/CodeGen/CGExpr.cpp:2937-2943
@@ -2936,2 +2936,9 @@
   QualType BaseTy = Base.getType();
+  // If the base is a pointer, we actually need to compute the offset in the
+  // pointee.
+  if (BaseTy->isAnyPointerType()) {
+    auto RBase = EmitLoadOfLValue(Base, E->getExprLoc());
+    Base = MakeNaturalAlignAddrLValue(
+        RBase.getScalarVal(), BaseTy->getAs<PointerType>()->getPointeeType());
+  }
   llvm::Value *Idx = nullptr;
----------------
ABataev wrote:
> This must be fixed already, please reabse
Thanks for the fix! I've  just posted a rebased patch.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3915-3946
@@ +3914,34 @@
+    // Reference types are ignored for mapping purposes.
+    if (auto *RefTy = ExprTy->getAs<ReferenceType>())
+      ExprTy = RefTy->getPointeeType().getCanonicalType();
+
+    // Given that an array section is considered a built-in type, we need to
+    // do the calculation based on the length of the section instead of relying
+    // on CGF.getTypeSize(E->getType()).
+    if (const auto *OAE = dyn_cast<OMPArraySectionExpr>(E)) {
+
+      auto BaseTy = OAE->getBase()->getType().getCanonicalType();
+      // Reference types are ignored for mapping purposes.
+      if (auto *RefTy = BaseTy->getAs<ReferenceType>())
+        BaseTy = RefTy->getPointeeType().getCanonicalType();
+
+      // If there is no length associated with the expression, that means we
+      // are using the whole length of the base.
+      if (!OAE->getLength())
+        return CGF.getTypeSize(BaseTy);
+
+      llvm::Value *ElemSize;
+      if (auto *PTy = BaseTy->getAs<PointerType>()) {
+        ElemSize = CGF.getTypeSize(PTy->getPointeeType().getCanonicalType());
+      } else {
+        auto *ATy = cast<ArrayType>(BaseTy.getTypePtr());
+        assert(ATy && "Expecting array type if not a pointer type.");
+        ElemSize = CGF.getTypeSize(ATy->getElementType().getCanonicalType());
+      }
+
+      auto *LengthVal = CGF.EmitScalarExpr(OAE->getLength());
+      LengthVal =
+          CGF.Builder.CreateIntCast(LengthVal, CGF.SizeTy, /*isSigned=*/false);
+      return CGF.Builder.CreateNUWMul(LengthVal, ElemSize);
+    }
+    return CGF.getTypeSize(ExprTy);
----------------
ABataev wrote:
> I don't think this is correct. It won't work for 2(and more)-dimensional array sections. Instead calculate size as 'last_item_address-first_item_address+1'.
I don't see why it should not work. There was only a small issue related with array decays to pointer that I fixed in the new diff. The size of the section always refer to the right most expression, even for multidimensional arrays. I added regression tests for that.

Other reason I avoided computing sizes based on addresses is that they do not get folded into constants. In code generation we have optimized behaviors for when all the sizes are constants so that a constant array is implemented instead of filling (at runtime) an array with constants to be passed  to the runtime library.

Let me know if you disagree.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4126-4128
@@ +4125,5 @@
+      if (auto *ME = dyn_cast<MemberExpr>(I->first)) {
+        // The base is the 'this' pointer. The content of the pointer is going
+        // to be the base of the field being mapped.
+        BP = CGF.EmitScalarExpr(ME->getBase());
+      } else {
----------------
ABataev wrote:
> In a few days you won't need it at all. All member references are implicitly captured into special declaration and you should not worry about them at all
Great! Thanks for working on that! I'll then see how that will interfere with the map clause code generation.


http://reviews.llvm.org/D16749