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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl 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;
----------------
ABataev wrote:
> I believe this function can be removed
done


================
Comment at: lib/Sema/SemaCUDA.cpp:616
       if (getLangOpts().CUDAIsDevice) {
-        return IsKnownEmitted(*this, dyn_cast<FunctionDecl>(CurContext))
+        return (getEmissionStatus(dyn_cast<FunctionDecl>(CurContext)) ==
+                FunctionEmissionStatus::Emitted)
----------------
ABataev wrote:
> I assume, better to use `cast` here, not `dyn_cast`
done


================
Comment at: lib/Sema/SemaCUDA.cpp:645
 
-      return IsKnownEmitted(*this, dyn_cast<FunctionDecl>(CurContext))
+      return (getEmissionStatus(dyn_cast<FunctionDecl>(CurContext)) ==
+              FunctionEmissionStatus::Emitted)
----------------
ABataev wrote:
> Same here, just `cast`
done


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


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


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


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


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


================
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.
----------------
ABataev wrote:
> Reformat the comment here
done


================
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");
----------------
ABataev wrote:
> The same condition is checked twice, one of them must be for `CalleeS`, I believe
done


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


================
Comment at: lib/Sema/SemaOpenMP.cpp:1627-1628
+  if (Caller) {
+    auto CallerS = getEmissionStatus(Caller);
+    auto CalleeS = getEmissionStatus(Callee);
+    assert(CallerS != FunctionEmissionStatus::CUDADiscarded &&
----------------
ABataev wrote:
> Better to use real type instead of `auto` here
done


================
Comment at: lib/Sema/SemaOpenMP.cpp:1670-1671
+  if (Caller) {
+    auto CallerS = getEmissionStatus(Caller);
+    auto CalleeS = getEmissionStatus(Callee);
+    assert(
----------------
ABataev wrote:
> Real types instead of `auto`
done


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

https://reviews.llvm.org/D67837