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

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked an inline comment as done.
gtbercea added inline comments.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:10364
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(FD)) {
+      HasEmittedDeclareTargetRegion = true;
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568