[PATCH] D38798: [OpenMP] Support for implicit "declare target" functions - Sema patch

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Tests?



================
Comment at: include/clang/Basic/LangOptions.def:193
 LANGOPT(OpenMPUseTLS      , 1, 0, "Use TLS for threadprivates or runtime calls")
+LANGOPT(OpenMPImplicitDeclareTarget , 1, 0, "Enable implicit declare target extension - marks automatically declarations and definitions with declare target attribute")
 LANGOPT(OpenMPIsDevice    , 1, 0, "Generate code only for OpenMP target device")
----------------
Where is it assigned a value 1? Currently it is disabled by default (initial value is 0).


================
Comment at: include/clang/Sema/SemaInternal.h:65
+// mode.
+inline bool DeclAttrsMatchOffloadMode(const LangOptions &LangOpts, Decl *D,
+                                      bool InOpenMPDeviceRegion) {
----------------
Function should start from small letter


================
Comment at: include/clang/Sema/SemaInternal.h:70
+
+  return DeclAttrsMatchCUDAMode(LangOpts, D);
+}
----------------
This function should be called only we're using CUDA NVPTX as an offloading device. Also, this function requires that `LangOpts.CUDA` was true. Do we set `LangOpts.CUDA` to trues when trying to offload to CUDE devices at all? If no, this function is useless.


================
Comment at: lib/Parse/ParseOpenMP.cpp:785-789
+      // Save the declarations so that we can create the declare target group
+      // later on.
+      if (Ptr)
+        for (auto *V : Ptr.get())
+          Decls.push_back(V);
----------------
Why do you need it?


================
Comment at: lib/Parse/ParseOpenMP.cpp:810-813
+    // If we have decls generate the group so that code can be generated for it
+    // later on.
+    if (!Decls.empty())
+      return Actions.BuildDeclaratorGroup(Decls);
----------------
Again, why do you need it?


================
Comment at: lib/Sema/SemaDecl.cpp:12661-12665
+  // In case of OpenMPImplicitDeclareTarget, semantically parsed function body
+  // is visited to mark inner callexpr with OMPDeclareTargetDeclAttr attribute.
+  if (getLangOpts().OpenMP && getLangOpts().OpenMPImplicitDeclareTarget)
+    checkDeclImplicitlyUsedOpenMPTargetContext(dcl);
+
----------------
I think this should be done only if the function has attached `OMPDeclareTargetDeclAttr` and if `!getLangOpts().IsOpenMPDevice`. Also, I don't like the idea of recursive scanning. It is better to add such markings on the fly (when trying to build the expression)


================
Comment at: lib/Sema/SemaOpenMP.cpp:1170
+
+/// Traverse declaration of /param D to check whether it has
+/// OMPDeclareTargetDeclAttr or not. If so, it marks definition with
----------------
`\param`


================
Comment at: lib/Sema/SemaOpenMP.cpp:1177-1178
+    // Revealed function calls are marked with OMPDeclareTargetDeclAttr
+    // attribute,
+    // in case -fopenmp-implicit-declare-target extension is enabled.
+    ImplicitDeviceFunctionChecker FunctionCallChecker(SemaRef);
----------------
Formatting


================
Comment at: lib/Sema/SemaOpenMP.cpp:1178
+    // attribute,
+    // in case -fopenmp-implicit-declare-target extension is enabled.
+    ImplicitDeviceFunctionChecker FunctionCallChecker(SemaRef);
----------------
There is no such option yet.


================
Comment at: lib/Sema/SemaOpenMP.cpp:1191
+
+  if (FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
+    if (FD->hasBody()) {
----------------
`auto *FD`


================
Comment at: lib/Sema/SemaOpenMP.cpp:1193
+    if (FD->hasBody()) {
+      for (auto RI : FD->redecls()) {
+        if (RI->hasAttr<OMPDeclareTargetDeclAttr>()) {
----------------
`const auto *RI`


================
Comment at: lib/Sema/SemaOpenMP.cpp:1194-1201
+        if (RI->hasAttr<OMPDeclareTargetDeclAttr>()) {
+          Attr *A = OMPDeclareTargetDeclAttr::CreateImplicit(
+              Context, OMPDeclareTargetDeclAttr::MT_To);
+          D->addAttr(A);
+
+          ImplicitDeclareTargetCheck(*this, FD);
+          return;
----------------
Could you reuse the same attribute attached to one of the redeclarations? 


================
Comment at: lib/Sema/SemaOpenMP.cpp:1212-1214
+      Attr *A = OMPDeclareTargetDeclAttr::CreateImplicit(
+          SemaRef.Context, OMPDeclareTargetDeclAttr::MT_To);
+      Class->addAttr(A);
----------------
Reuse the existing attribute


================
Comment at: lib/Sema/SemaOpenMP.cpp:1233-1235
+  if (FunctionDecl *Callee = Call->getDirectCallee()) {
+    return VisitFunctionDecl(Callee);
+  }
----------------
Remove braces


================
Comment at: lib/Sema/SemaOpenMP.cpp:1245-1247
+  const RecordType *RT =
+      SemaRef.Context.getBaseElementType(Ty)->getAs<RecordType>();
+  CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());
----------------
`const auto *RD = SemaRef.Context.getBaseElementType(Ty)->getAsCXXRecordDecl();`


Repository:
  rL LLVM

https://reviews.llvm.org/D38798