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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2343-2344
+    // pointer
+    if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+      Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
     KLoc.push_back(Tok.getLocation());
----------------
Is this possible at all? I think it must an assertion rather this kind of trick.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:651
+    if (DMVC == DMVC_scalar || DMVC == DMVC_pointer) {
+      return (getDefaultDMIBAtLevel(Level, DMVC) ==
+                DMIB_alloc) ||
----------------
Cache the result of `getDefaultDMIBAtLevel(Level, DMVC)` in a var, do not call it several times.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:663
+  bool isDefaultmapDefaultBehavior(DefaultMapVariableCategory DMVC) const {
+    return (getDefaultDMIB(DMVC) == DMIB_unspecified) ||
+           (getDefaultDMIB(DMVC) == DMIB_firstprivate) ||
----------------
Same, cache the result of the call


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:668
+  bool isDefaultmapDefaultBehaviorAtLevel(unsigned Level,
+                                   DefaultMapVariableCategory DMVC) const {
+    return (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_unspecified) ||
----------------
Seems to me, the code is not formatted. Use clang-format.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:669
+                                   DefaultMapVariableCategory DMVC) const {
+    return (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_unspecified) ||
+           (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_firstprivate) ||
----------------
same here


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2843
   llvm::SmallVector<Expr *, 4> ImplicitFirstprivate;
-  llvm::SmallVector<Expr *, 4> ImplicitMap;
+  llvm::SmallVector<Expr *, 4> ImplicitMap[DMIB_none];
   Sema::VarsWithInheritedDSAType VarsWithInheritedDSA;
----------------
I would prefer to use `OpenMPMapClauseKind` type as a key here to prevent all those conversions of the default (none, default, unknown) mappings to real mappings.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2987
           if (IsFirstprivate)
             ImplicitFirstprivate.emplace_back(E);
+          else {
----------------
Enclose this code into braces too, if the code in `else` branch is enclosed in braces the code in `then` branch also must be enclosed in braces.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2990
+            DefaultMapImplicitBehavior DMIB = Stack->getDefaultDMIB(DMVC);
+            if (DMIB >= DMIB_none) {
+              if (DMVC == DMVC_aggregate || Res) DMIB = DMIB_tofrom;
----------------
Better to use switches rather such kind of comparisons for enums.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2991-2992
+            if (DMIB >= DMIB_none) {
+              if (DMVC == DMVC_aggregate || Res) DMIB = DMIB_tofrom;
+              else llvm_unreachable("Unexpected defaultmap implicit behavior");
+            }
----------------
Format the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204