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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits