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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/Sema/SemaCUDA.cpp:604
 // Do we know that we will eventually codegen the given function?
 static bool IsKnownEmitted(Sema &S, FunctionDecl *FD) {
+  return S.getEmissionStatus(FD) == Sema::FunctionEmissionStatus::Emitted;
----------------
I believe this function can be removed


================
Comment at: lib/Sema/SemaDecl.cpp:17619
+
+  auto OMPES = FunctionEmissionStatus::Unknown;
+  if (LangOpts.OpenMPIsDevice) {
----------------
Better to use real type here, not `auto`


================
Comment at: lib/Sema/SemaDecl.cpp:17623-17626
+    if (DevTy.hasValue())
+      OMPES = (*DevTy == OMPDeclareTargetDeclAttr::DT_Host)
+               ? FunctionEmissionStatus::OMPDiscarded
+               : FunctionEmissionStatus::Emitted;
----------------
Enclose in braces


================
Comment at: lib/Sema/SemaDecl.cpp:17626
+               ? FunctionEmissionStatus::OMPDiscarded
+               : FunctionEmissionStatus::Emitted;
+  } else if (LangOpts.OpenMP) {
----------------
Hmm, it must be marked as know-emitted only if `S.DeviceKnownEmittedFns.count(FD) > 0`. Otherwise, it must be unknown.


================
Comment at: lib/Sema/SemaDecl.cpp:17629-17631
+    if (LangOpts.OpenMP <= 45)
+      OMPES = FunctionEmissionStatus::Emitted;
+    else {
----------------
Enclose in braces, not goo to have `else` branch enclosed in braces and `then` branch without.


================
Comment at: lib/Sema/SemaDecl.cpp:17641
+                 ? FunctionEmissionStatus::OMPDiscarded
+                 : FunctionEmissionStatus::Emitted;
+    }
----------------
Same here, it must be marked as know-emitted only if `S.DeviceKnownEmittedFns.count(FD) > 0`. Otherwise, it must be unknown.


================
Comment at: lib/Sema/SemaDecl.cpp:17684-17689
+  // If we have
+  //   host fn calls kernel fn calls host+device,
+  // the HD function does not get instantiated on the host.  We model this by
+  // omitting at the call to the kernel from the callgraph.  This ensures
+  // that, when compiling for host, only HD functions actually called from the
+  // host get marked as known-emitted.
----------------
Reformat the comment here


================
Comment at: lib/Sema/SemaOpenMP.cpp:1629-1630
+    auto CalleeS = getEmissionStatus(Callee);
+    assert(CallerS != FunctionEmissionStatus::CUDADiscarded &&
+           CallerS != FunctionEmissionStatus::CUDADiscarded &&
+           "CUDADiscarded unexpected in OpenMP device function check");
----------------
The same condition is checked twice, one of them must be for `CalleeS`, I believe


================
Comment at: lib/Sema/SemaOpenMP.cpp:1674
+        (LangOpts.CUDA || (CallerS != FunctionEmissionStatus::CUDADiscarded &&
+                           CallerS != FunctionEmissionStatus::CUDADiscarded)) &&
+        "CUDADiscarded unexpected in OpenMP host function check");
----------------
Again, the same condition checked twice.


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

https://reviews.llvm.org/D67837