[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime

[Alexey Bataev via Phabricator via cfe-commits]

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