[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:9291
+  if (CGM.getLangOpts().OpenMPSimd || CGM.getLangOpts().OpenMPIsDevice ||
+      (OffloadEntriesInfoManager.empty() && !HasEmittedDeclareTargetRegion))
+    return nullptr;
----------------
Missed check for `HasEmittedTargetRegion` here


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9312
+    // contain at least 1 target region.
+    if ((HasEmittedTargetRegion || HasEmittedDeclareTargetRegion) &&
+        HasRequiresUnifiedSharedMemory)
----------------
The first part of the condition is excessive. You have early exit from the function. Instead of the condition, use `assert((!OffloadEntriesInfoManager.empty() || HasEmittedDeclareTargetRegion || HasEmittedTargetRegion) && "Expected bla-bla-bla");`


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:10364
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(FD)) {
+      HasEmittedDeclareTargetRegion = true;
----------------
gtbercea wrote:
> ABataev wrote:
> > gtbercea wrote:
> > > ABataev wrote:
> > > > gtbercea wrote:
> > > > > ABataev wrote:
> > > > > > ABataev wrote:
> > > > > > > No need for the braces
> > > > > > What if `declare target` is used only for variabes but not for the functions?
> > > > > Even more reason to error in that case since it may contain clauses like link or to which need for requires directives to be used consistently.
> > > > But I don't see that your patch works in this situation. Currently, it will emit the error only if the declare target function is found, no?
> > > Actually it will work even when just variables are used in the declare target region. There is another problem which I have a fix for. I will update the patch in a bit. 
> > Why will it work in this case? If you have just a declare target region in the code for the variables and nothing else. You don't have target regions, declare target functions etc. It won't work in this case.
> It will work because the OffloadEntriesInfoManager.empty() will return false in that case.
But you don't have a check for `OffloadEntriesInfoManager.empty() ` when you set `Flags` for `__tgt_register_requires` function call.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568