[PATCH] D67837: [CUDA][HIP] Fix host/device check with -fopenmp

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 12 inline comments as done.
yaxunl added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:17618
+
+  if (LangOpts.CUDA) {
+    // When compiling for device, host functions are never emitted.  Similarly,
----------------
ABataev wrote:
> Are you going to handle `#pragma omp declare target device_type(nohost)` in Cuda mode correctly? Such functions should not be emitted on the host in OpenMP 5.0.
will do


================
Comment at: lib/Sema/SemaOpenMP.cpp:1569-1574
+static Sema::FunctionEmissionStatus isKnownDeviceEmitted(Sema &S,
+                                                         FunctionDecl *FD) {
   assert(S.LangOpts.OpenMP && S.LangOpts.OpenMPIsDevice &&
          "Expected OpenMP device compilation.");
-  // Templates are emitted when they're instantiated.
-  if (FD->isDependentContext())
-    return FunctionEmissionStatus::Discarded;
-
-  Optional<OMPDeclareTargetDeclAttr::DevTypeTy> DevTy =
-      OMPDeclareTargetDeclAttr::getDeviceType(FD->getCanonicalDecl());
-  if (DevTy.hasValue())
-    return (*DevTy == OMPDeclareTargetDeclAttr::DT_Host)
-               ? FunctionEmissionStatus::Discarded
-               : FunctionEmissionStatus::Emitted;
-
-  // Otherwise, the function is known-emitted if it's in our set of
-  // known-emitted functions.
-  return (S.DeviceKnownEmittedFns.count(FD) > 0)
-             ? FunctionEmissionStatus::Emitted
-             : FunctionEmissionStatus::Unknown;
+  return S.getEmissionStatus(FD);
 }
----------------
ABataev wrote:
> You can remove the whole function and use `Sema::getEmissionStatus()` instead.
done


================
Comment at: lib/Sema/SemaOpenMP.cpp:1593
+  case FunctionEmissionStatus::OMPDiscarded:
+  case FunctionEmissionStatus::CUDADiscarded:
     Kind = DeviceDiagBuilder::K_Nop;
----------------
ABataev wrote:
> I think, `CUDADiscarded` case must be unreachable and it must be a case for `llvm_unreachable()` in case of the OpenMP device code.
done


================
Comment at: lib/Sema/SemaOpenMP.cpp:1602-1607
+static Sema::FunctionEmissionStatus isKnownHostEmitted(Sema &S,
+                                                       FunctionDecl *FD) {
   assert(S.LangOpts.OpenMP && !S.LangOpts.OpenMPIsDevice &&
          "Expected OpenMP host compilation.");
-  // In OpenMP 4.5 all the functions are host functions.
-  if (S.LangOpts.OpenMP <= 45)
-    return FunctionEmissionStatus::Emitted;
-
-  Optional<OMPDeclareTargetDeclAttr::DevTypeTy> DevTy =
-      OMPDeclareTargetDeclAttr::getDeviceType(FD->getCanonicalDecl());
-  if (DevTy.hasValue())
-    return (*DevTy == OMPDeclareTargetDeclAttr::DT_NoHost)
-               ? FunctionEmissionStatus::Discarded
-               : FunctionEmissionStatus::Emitted;
-
-  // Otherwise, the function is known-emitted if it's in our set of
-  // known-emitted functions.
-  return (S.DeviceKnownEmittedFns.count(FD) > 0)
-             ? FunctionEmissionStatus::Emitted
-             : FunctionEmissionStatus::Unknown;
+  return S.getEmissionStatus(FD);
 }
----------------
ABataev wrote:
> You can remove the whole function and use `Sema::getEmissionStatus()` instead.
done


================
Comment at: lib/Sema/SemaOpenMP.cpp:1647-1648
             FunctionEmissionStatus::Unknown)) &&
       isKnownDeviceEmitted(*this, Callee) ==
-          FunctionEmissionStatus::Discarded) {
+          FunctionEmissionStatus::OMPDiscarded) {
     StringRef HostDevTy =
----------------
ABataev wrote:
> I would add an assert thet this function does not return `CUDADiscarded` value.
done


================
Comment at: lib/Sema/SemaOpenMP.cpp:1684-1685
       isKnownHostEmitted(*this, Caller) == FunctionEmissionStatus::Emitted &&
-      isKnownHostEmitted(*this, Callee) == FunctionEmissionStatus::Discarded) {
+      isKnownHostEmitted(*this, Callee) ==
+          FunctionEmissionStatus::OMPDiscarded) {
     StringRef NoHostDevTy = getOpenMPSimpleClauseTypeName(
----------------
ABataev wrote:
> Also, it would be good to check that it cannot return `CUDADiscarded` here.
In host compilation, if the source code is CUDA/HIP language, a CUDA/HIP device function may be discarded as CUDADiscarded and show up here. For C++, CUDADiscarded should not be returned here.


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

https://reviews.llvm.org/D67837