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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/Basic/OpenMPKinds.def:426-436
+OPENMP_DEFAULTMAP_KIND(aggregate)
+OPENMP_DEFAULTMAP_KIND(pointer)
 
 // Modifiers for 'defaultmap' clause.
+OPENMP_DEFAULTMAP_MODIFIER(alloc)
+OPENMP_DEFAULTMAP_MODIFIER(to)
+OPENMP_DEFAULTMAP_MODIFIER(from)
----------------
Add some guards in the code, these values must be enabled only for OpenMP 5.0, for 4.5 only scalar:tofrom is allowed. Add a test that the error messages are emitted for new cases in OpenMP 4.5


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7274
+      case OMPC_DEFAULTMAP_MODIFIER_none:
+        Bits = OMP_MAP_NONE;
+        break;
----------------
I think you just need to keep the original value of the `Bits` variable set in line 7242


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7276
+        break;
+      default:
+        llvm_unreachable("Unexpected implicit behavior!");
----------------
It is not recommended to use `default:` case in switches over enumerics, just add a case for each enum value and remove `default:`


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7368-7370
+      bool IsImplicit, OpenMPDefaultmapClauseModifier ImplicitBehavior =
+          OMPC_DEFAULTMAP_MODIFIER_default,
+      OpenMPDefaultmapClauseKind VariableCategory = OMPC_DEFAULTMAP_unknown,
----------------
Do you really need the default params values here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:134
+    DefaultMapVariableCategory VariableCategory = DMVC_unspecified;
+    SourceLocation SLoc = SourceLocation();
+    DefaultmapInfo() = default;
----------------
No need to call the constructor for classes as a default value, just `SourceLocation SLoc;` is enough


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
     SourceLocation DefaultAttrLoc;
-    DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-    SourceLocation DefaultMapAttrLoc;
+    DefaultmapInfo DefaultmapMap[3];
+
----------------
Maybe, it would be better to make `DMVC_unspecified` the last one in the `DefaultMapVariableCategory` and use it as an array dimension here rather than rely on the magical number?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:623
+    auto &DefaultmapInfo = getTopOfStack().
+                           DefaultmapMap[static_cast<int>(DMVC-1)];
+    DefaultmapInfo.ImplicitBehavior = DMIB;
----------------
Do you really need the static_cast here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:625
+    DefaultmapInfo.ImplicitBehavior = DMIB;
+    DefaultmapInfo.VariableCategory = DMVC;
+    DefaultmapInfo.SLoc = Loc;
----------------
Do you really need this field if your DefaultmapMap already variable category based array?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:630
+  bool hasSetDefaultmapCategory(OpenMPDefaultmapClauseKind VariableCategory) {
+    int VC = static_cast<int>(VariableCategory);
+    return getTopOfStack().DefaultmapMap[VC].ImplicitBehavior != DMIB_unspecified &&
----------------
Do you really need a cast here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:656
+                                 DefaultMapVariableCategory DMVC) const {
+    if (DMVC == DMVC_scalar) {
+      return (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
----------------
Better to use switch here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:657-673
+      return (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+                DMIB_alloc) ||
+             (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+                DMIB_to) ||
+             (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+                DMIB_from) ||
+             (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
----------------
Seems to me, these 2 checks are very similar, you cam merge them


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2928
+        bool IsDMIBNone = false;
+        if (VD->getType()->isPointerType() || VD->getType()->isMemberPointerType()) {
+          IsDMIBNone =
----------------
Just `isAnyPointerType()`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2932-2933
+        } else if (VD->getType()->isScalarType() &&
+                   !VD->getType()->isBlockPointerType() &&
+                   !VD->getType()->isObjCObjectPointerType()) {
+          IsDMIBNone =
----------------
Just `!VD->getType()->isAnyPointerType()`. Plus, I think you won't need it here in case of fixed condition in the main `if`.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2936
+            Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) == DMIB_none;
+        } else if (VD->getType()->isArrayType() || VD->getType()->isRecordType()) {
+          IsDMIBNone =
----------------
Just `isAggregateType()`? 


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2996-3003
+                    DMIB_unspecified ||
+                  Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) ==
+                    DMIB_default)) ||
+                  (VD->getType().getNonReferenceType()->isPointerType() &&
+                   (Stack->getDefaultDMIB(OMPC_DEFAULTMAP_pointer) ==
+                    DMIB_unspecified ||
+                    Stack->getDefaultDMIB(OMPC_DEFAULTMAP_pointer) ==
----------------
Also must include DMIB_firstprivate.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16365
+    if (M != OMPC_DEFAULTMAP_MODIFIER_to && M != OMPC_DEFAULTMAP_MODIFIER_from &&
+      M != OMPC_DEFAULTMAP_MODIFIER_tofrom && M != OMPC_DEFAULTMAP_MODIFIER_firstprivate &&
+      M != OMPC_DEFAULTMAP_MODIFIER_none && M != OMPC_DEFAULTMAP_MODIFIER_default) {
----------------
The code is not formatted.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16386
+
+  if (Kind == OMPC_DEFAULTMAP_scalar) {
+    switch (M) {
----------------
Better to use switch here too


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:16409
+        break;
+      default:
+        llvm_unreachable("Unknown OMPC_DEFAULTMAP_MODIFIER");
----------------
No default


================
Comment at: clang/test/OpenMP/target_defaultmap_codegen.cpp:44
+  // CK1: call void [[KERNEL:@.+]]({ double, double }* [[PTR]])
+  #pragma omp target defaultmap(alloc:scalar)
+  {
----------------
Why it is allowed in OpenMP 4.5? I don't see an option in your tests for OpenMP 5.0. In 4.5 there must be an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69204