[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/CGOpenMPRuntime.cpp:8970
+    if (Clause->getClauseKind() == OMPC_unified_shared_memory)
+      CGM.getOpenMPRuntime().HasRequiresUnifiedSharedMemory = true;
+  }
----------------
You can do `break;` here, no need for further analysis


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9050
+    CGF.StartFunction(GlobalDecl(), C.VoidTy, RequiresRegFn, FI, {});
+    int64_t Flags = OMP_REQ_NONE;
+    //TODO: check for other requires clauses.
----------------
Change the type from `int64_t` to `OpenMPOffloadingRequiresDirFlags`


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8978
+    if (Clause->getClauseKind() == OMPC_unified_shared_memory) {
+      CGM.getOpenMPRuntime().HasRequiresUnifiedSharedMemory = true;
+      break;
----------------
ABataev wrote:
> Just `HasRequiresUnifiedSharedMemory = true;`
Not done


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:641
+  /// directive is present.
+  bool HasRequiresUnifiedSharedMemory = false;
+
----------------
AlexEichenberger wrote:
> Don't you need a bool for each characteristics? Your intention is to have one bit vector for each characteristics that matter to the compiler?
> 
> Also, it is my belief that you need to record 2 states: 
> unmaterialized (meaning I have not processed any target yet, so I should record any requires as they arrive)
> finalized (I am processing a target, so the state is now fixed)
> 
> you need this in case you have an input like this:
> 
> declare target
> int x
> end declare target
> 
> requires unified memory
> 
> which is illegal
What about this comment?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:1438
+  /// requires directives was used in the current module.
+  virtual llvm::Function *emitRequiresDirectiveRegFun();
+
----------------
ABataev wrote:
> Why do you need this vertual funtion? I think static local is going to be enough
Can you make it `const` member function?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568