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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
