[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:137-140
+             SourceLocation Loc) {
+      ImplicitBehavior = IB;
+      VariableCategory = VC;
+      SLoc = Loc;
----------------
Use member initializers in constructors.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:152
     SourceLocation DefaultAttrLoc;
-    DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-    SourceLocation DefaultMapAttrLoc;
+    std::array<DefaultmapInfo, 3> DefaultmapMap;
+
----------------
No, I meant just a C array, like `DefaultmapInfo DefaultMap[3];`.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:620-624
+  /// Set default data mapping attribute to 'alloc:scalar'.
+  void setDefaultDMAAllocScalar(SourceLocation Loc) {
+    getTopOfStack().DefaultmapMap[OMPC_DEFAULTMAP_scalar]
+      .set(DMIB_alloc, DMVC_scalar, Loc);
+  }
----------------
Why do we need so many functions? Better to have just one function with additional params.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1967
           (DSAStack->isForceCaptureByReferenceInTargetExecutable() &&
            !Ty->isAnyPointerType()) ||
           !Ty->isScalarType() ||
----------------
Seems to me, you're missing additional checks for pointers here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1969-1972
+          (DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+           DMIB_alloc ||
+           DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+           DMIB_to ||
----------------
I think, alloc and to scalars also must be captured by value. Moreover, seems to me, alloc scalars should not be captured at all since their behavior is very similar to the behavior of private variables.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1978
+           DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+           DMIB_firstprivate) ||
           DSAStack->hasExplicitDSA(
----------------
If the scalars are mapped as firstprivates by default, they must be captured by value.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2200-2201
+              DMIB_tofrom &&
+          DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_scalar) !=
+              DMIB_firstprivate) &&
+          !(D->getType()->isPointerType() && // TODO check
----------------
Seems to me, this is wrong. If the default is firstprivate the variables must be marked as firstprivate. Maybe, just check if the default for scalars is not specified or is firstprivate instead of so many checks?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2202-2212
+          !(D->getType()->isPointerType() && // TODO check
+          (DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+              DMIB_alloc ||
+          DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+              DMIB_to ||
+          DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+              DMIB_from ||
----------------
1. Also, check the logic here. 
2. Why there is TODO?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3059-3082
+              (((VD->getType().getNonReferenceType()->isScalarType() &&
+               Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+                 DMIB_alloc &&
+               Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+                 DMIB_to &&
+               Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+                 DMIB_from &&
----------------
I see similar cehcks in different places, Outline them into a standalone function.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4466-4472
+
   if (AStmt && !CurContext->isDependentContext()) {
     assert(isa<CapturedStmt>(AStmt) && "Captured statement expected");
 
     // Check default data sharing attributes for referenced variables.
     DSAAttrChecker DSAChecker(DSAStack, *this, cast<CapturedStmt>(AStmt));
+
----------------
Extra blank lines


================
Comment at: clang/lib/Sema/TreeTransform.h:1987-2001
+  /// Build a new OpenMP 'defaultmap' clause.
+  ///
+  /// By default, performs semantic analysis to build the new OpenMP clause.
+  /// Subclasses may override this routine to provide different behavior.
+  OMPClause *RebuildOMPDefaultmapClause(OpenMPDefaultmapClauseModifier M,
+                                        OpenMPDefaultmapClauseKind Kind,
+                                        SourceLocation StartLoc,
----------------
Do you really need to rebuild it? It has only constants inside.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204