ABataev added inline comments.
================ Comment at: lib/CodeGen/CGDecl.cpp:2576 void CodeGenModule::EmitOMPRequiresDecl(const OMPRequiresDecl *D) { - getOpenMPRuntime().checkArchForUnifiedAddressing(D); } ---------------- You don't need to pass the reference to CodeGenModule to CGOpenMPRuntime class, it handles a reference to the CodeGenMode already. ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:726 +enum OpenMPOffloadingRequiresDirFlags : int64_t { + /// no requires directive present. ---------------- You must use `LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();` and `LLVM_MARK_AS_BITMASK_ENUM(/* LargestFlag = */)` from `clang/Basic/BitmaskEnum.h` to enable bit operations on your new bit-arithmetic based enumeric. ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:726 +enum OpenMPOffloadingRequiresDirFlags : int64_t { + /// no requires directive present. ---------------- ABataev wrote: > You must use `LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();` and > `LLVM_MARK_AS_BITMASK_ENUM(/* LargestFlag = */)` from > `clang/Basic/BitmaskEnum.h` to enable bit operations on your new > bit-arithmetic based enumeric. Add a comment for the whole new type ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:728 + /// no requires directive present. + OMP_REQ_NONE = 0x000, + /// reverse_offload clause. ---------------- You should not handle all the possible flags for the requires directives, since you try to support only target-specific flags. ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:2322 + auto *FnTy = + llvm::FunctionType::get(CGM.Int32Ty, TypeParams, /*isVarArg*/ false); + RTLFn = CGM.CreateRuntimeFunction(FnTy, "__tgt_register_requires"); ---------------- Function return type must be `void` ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3870 llvm::Function * +CGOpenMPRuntime::createRequiresDirectiveRegistration() { + // If we don't have entries or if we are emitting code for the device, we ---------------- Why do you need a new member function? Can you make a static local function? ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3882 + const auto &FI = + CGM.getTypes().arrangeBuiltinFunctionDeclaration(C.VoidTy, {}); + llvm::FunctionType *FTy = CGM.getTypes().GetFunctionType(FI); ---------------- Use `getTypes().arrangeNullaryFunction()` ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3887 + CGF.StartFunction(GlobalDecl(), C.VoidTy, RequiresRegFn, FI, {}); + int64_t Flags = OMP_REQ_NONE; + //TODO: check for other requires clauses. ---------------- Use `OpenMPOffloadingRequiresDirFlags` instead of `int64_t` ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7983 MapFlagsArrayTy &Types) const { + // If using unified memory, no need to do the mappings. + if (CGF.CGM.HasRequiresUnifiedSharedMemory) ---------------- Seems to me, this must be in another patch, has nothing to do with this patch ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9069 +llvm::Function *CGOpenMPRuntime::emitRequiresDirectiveRegFun() { + // Create and register the function that handles the requires directives. ---------------- Why do you need the second function? ================ Comment at: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:4946 + CodeGenModule &CGM, const OMPRequiresDecl *D) const { + CGOpenMPRuntime::checkArchForUnifiedAddressing(CGM, D); for (const OMPClause *Clause : D->clauselists()) { ---------------- Call this function after the target-specific processing. ================ Comment at: lib/CodeGen/CodeGenModule.h:294 + bool HasRequiresUnifiedSharedMemory = false; + ---------------- Why public? Must be private. Also, add comments for this new data member. Also, I don't think this must be in CGM. It must be a member of CGOpenMPRuntime class. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60568/new/ https://reviews.llvm.org/D60568 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits