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

[Alexey Bataev via cfe-commits]

ABataev added inline comments.

================
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!");
+      }
+    }
----------------
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?

================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:4462
@@ +4461,3 @@
+    if (const auto *OAE = dyn_cast<OMPArraySectionExpr>(E)) {
+      auto BaseTy = OMPArraySectionExpr::getBaseOriginalType(
+                        OAE->getBase()->IgnoreParenImpCasts())
----------------
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

================
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
----------------
Do we really need this? It is quite simple and has nothing to do with the name of the function

================
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
----------------
CGF has EmitLoadOfPointer() function

================
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
----------------
Maybe it is better to extract it as a member function?

================
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);
----------------
auto &&

================
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(),
----------------
getVarRefs()->varlists(). getVarRefs() is protected


http://reviews.llvm.org/D16749