[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7043-7044
     OMP_MAP_CLOSE = 0x400,
+    /// Produce a runtime error if the data is not already allocated.
+    OMP_MAP_PRESENT = 0x800,
     /// The 16 MSBs of the flags indicate whether the entry is member of some
----------------
ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > Better to use thу next value to avoid compatibility issues with XLC. 
> > You mean 0x1000?
> Yes
Could you add a comment that 0x800 is currently reserved for compatibility, please?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7934-7940
+    // If any element has the present modifier, then make sure the runtime
+    // doesn't attempt to allocate the struct.
+    if (CurTypes.end() !=
+        llvm::find_if(CurTypes, [](OpenMPOffloadMappingFlags Type) {
+          return Type & OMP_MAP_PRESENT;
+        }))
+      Types.back() |= OMP_MAP_PRESENT;
----------------
Why do we need this extra stuff here?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+    return OMPC_MAP_MODIFIER_unknown;
+  return static_cast<OpenMPMapModifierKind>(Type);
----------------
Why do we need this?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3133
+      Diag(Tok, diag::err_omp_unknown_map_type_modifier)
+          << (getLangOpts().OpenMP >= 51);
       ConsumeToken();
----------------
Better to use `(getLangOpts().OpenMP >= 51 ? 1 : 0)`, the `select` is of integer type.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:3150-3151
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type > OMPC_MAP_unknown)
+    return OMPC_MAP_unknown;
+  return static_cast<OpenMPMapClauseKind>(Type);
----------------
Same, why do we need this?


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

https://reviews.llvm.org/D83061