[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/CGOpenMPRuntime.cpp:4393-4431
@@ +4392,41 @@
+    /// retrieved from the provided map clause expression.
+    DeclarationMapInfoEntry(const Expr *MCE, OpenMPMapClauseKind MapType,
+                            OpenMPMapClauseKind MapTypeModifier)
+        : MapType(MapType), MapTypeModifier(MapTypeModifier) {
+      assert(MCE && "Invalid expression??");
+      while (true) {
+        MCE = MCE->IgnoreParenImpCasts();
+
+        if (auto *CurE = dyn_cast<DeclRefExpr>(MCE)) {
+          Components.push_back(
+              ComponentTy(CurE, cast<VarDecl>(CurE->getDecl())));
+          break;
+        }
+
+        if (auto *CurE = dyn_cast<MemberExpr>(MCE)) {
+          auto *BaseE = CurE->getBase()->IgnoreParenImpCasts();
+
+          Components.push_back(
+              ComponentTy(CurE, cast<FieldDecl>(CurE->getMemberDecl())));
+          if (isa<CXXThisExpr>(BaseE))
+            break;
+
+          MCE = BaseE;
+          continue;
+        }
+
+        if (auto *CurE = dyn_cast<ArraySubscriptExpr>(MCE)) {
+          Components.push_back(ComponentTy(CurE, nullptr));
+          MCE = CurE->getBase()->IgnoreParenImpCasts();
+          continue;
+        }
+
+        if (auto *CurE = dyn_cast<OMPArraySectionExpr>(MCE)) {
+          Components.push_back(ComponentTy(CurE, nullptr));
+          MCE = CurE->getBase()->IgnoreParenImpCasts();
+          continue;
+        }
+
+        llvm_unreachable("Invalid map clause expression!");
+      }
+    }
----------------
ABataev wrote:
> I do recall that similar code already exists in Sema. Maybe you'd better to store info about top-level bases in map clause in Sema rather than copy the code between modules?
Yes, Sema does something similar. In the last diff I use what Sema produces and store it in the clause. The implementation is in the dependence diff.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4462
@@ +4461,3 @@
+    if (const auto *OAE = dyn_cast<OMPArraySectionExpr>(E)) {
+      auto BaseTy = OMPArraySectionExpr::getBaseOriginalType(
+                        OAE->getBase()->IgnoreParenImpCasts())
----------------
ABataev wrote:
> Replace 'auto' by 'QualType'. Also, I think all such deep digging into structure of expressions must reside in Sema rather than in codegen and info must be stored within OMPMapClause
Done. The information is now stored in the map clause.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4493-4498
@@ +4492,8 @@
+
+  /// \brief Generate the address of the lower bound of the section defined by
+  /// expression \a E.
+  llvm::Value *getLowerBoundOfElement(const Expr *E) const {
+    return CGF.EmitLValue(E).getPointer();
+  }
+
+  /// \brief Return the corresponding bits for a given map clause modifier. Add
----------------
ABataev wrote:
> Do we really need this? It is quite simple and has nothing to do with the name of the function
Ok, I removed this.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4681-4684
@@ +4680,6 @@
+        if (I->second->getType()->isAnyPointerType() && std::next(I) != CE) {
+          auto PtrAddr =
+              CGF.MakeNaturalAlignAddrLValue(BP, I->second->getType());
+          BP = CGF.EmitLoadOfLValue(PtrAddr, SourceLocation()).getScalarVal();
+
+          // We do not need to generate individual map information for the
----------------
ABataev wrote:
> CGF has EmitLoadOfPointer() function
Ok, using `EmitLoadOfPointerLValue` now.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4699-4735
@@ +4698,39 @@
+
+        // A final array section, is one whose length can't be proved to be one.
+        auto IsFinalArraySection = [this](const Expr *E) -> bool {
+          auto *OASE = dyn_cast<OMPArraySectionExpr>(E);
+
+          // It is not an array section and therefore not a unity-size one.
+          if (!OASE)
+            return false;
+
+          // An array section with no colon always refer to a single element.
+          if (OASE->getColonLoc().isInvalid())
+            return false;
+
+          auto *Length = OASE->getLength();
+
+          // If we don't have a length we have to check if the array has size 1
+          // for this dimension. Also, we should always expect a length if the
+          // base type is pointer.
+          if (!Length) {
+            auto BaseQTy = OMPArraySectionExpr::getBaseOriginalType(
+                               OASE->getBase()->IgnoreParenImpCasts())
+                               .getCanonicalType();
+            if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr()))
+              return ATy->getSize().getSExtValue() != 1;
+            // If we don't have a constant dimension length, we have to consider
+            // the current section as having any size, so it is not necessarily
+            // unitary. If it happen to be unity size, that's user fault.
+            return true;
+          }
+
+          // Check if the length evaluates to 1.
+          llvm::APSInt ConstLength;
+          if (!Length->EvaluateAsInt(ConstLength, CGF.getContext()))
+            return true; // Can have more that size 1.
+
+          return ConstLength.getSExtValue() != 1;
+        }(I->first);
+
+        // Get information on whether the element is a pointer. Have to do a
----------------
ABataev wrote:
> Maybe it is better to extract it as a member function?
Moved this code to a new member function as suggested.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4700
@@ +4699,3 @@
+        // A final array section, is one whose length can't be proved to be one.
+        auto IsFinalArraySection = [this](const Expr *E) -> bool {
+          auto *OASE = dyn_cast<OMPArraySectionExpr>(E);
----------------
ABataev wrote:
> auto &&
I am now using a member function as you suggested below.

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4794
@@ +4793,3 @@
+    for (auto *MC : Directive.getClausesOfKind<OMPMapClause>()) {
+      for (auto *RE : MC->getVarRefs()) {
+        auto *MI = new DeclarationMapInfoEntry(RE, MC->getMapType(),
----------------
ABataev wrote:
> getVarRefs()->varlists(). getVarRefs() is protected
I am doing as you say. In any case,  the const variant of `getVarRefs` is currently public member. Maybe that is something you plan to fix?


http://reviews.llvm.org/D16749