From eda65f5d48dd3428a369384d52486b22377d9c9d Mon Sep 17 00:00:00 2001 From: Premanand M Rao Date: Wed, 12 May 2021 14:17:05 -0700 Subject: [PATCH 1/4] [SYCL] Emit diagnostics appropriately when coexisting with OpenMP In the presence of OpenMP, calls to undefined functions get diagnosed incorrectly in SYCL. With this change, the diagnostic is emitted only when the reason for the emission is SYCL. This change leverages the diagnostic "reason" infrastructure implemented in PR #3511. Signed-off-by: Premanand M Rao --- clang/include/clang/Sema/Sema.h | 26 ++++++++++++++- clang/lib/Sema/Sema.cpp | 2 +- clang/lib/Sema/SemaDecl.cpp | 13 ++++++-- clang/lib/Sema/SemaSYCL.cpp | 32 ++++++++++++++----- .../SemaSYCL/call-to-undefined-function.cpp | 4 +++ 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 056d4ad7bb341..70245d26b86e7 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1809,6 +1809,29 @@ class Sema final { LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/All) }; +private: + // A collection of a pair of undefined functions and their callers known + // to be reachable from a routine on the device (kernel or device function). + typedef std::pair CallPair; + llvm::SmallVector UndefinedReachableFromSyclDevice; + +public: + // Helper routine to add a pair of Callee-Caller pair of FunctionDecl * + // to UndefinedReachableFromSyclDevice. + void addFDToReachableFromSyclDevice(FunctionDecl *Callee, + FunctionDecl* Caller) { + UndefinedReachableFromSyclDevice.push_back(std::make_pair(Callee, Caller)); + } + // Helper routine to check if a pair of Callee-Caller FunctionDecl * + // is in UndefinedReachableFromSyclDevice. + bool isFDReachableFromSyclDevice(const FunctionDecl *Callee, + const FunctionDecl *Caller) { + for (auto It: UndefinedReachableFromSyclDevice) + if (It.first == Callee && It.second == Caller) + return true; + return false; + } + /// A generic diagnostic builder for errors which may or may not be deferred. /// /// In CUDA, there exist constructs (e.g. variable-length arrays, try/catch) @@ -13288,7 +13311,8 @@ class Sema final { /// properly declared for device compilation. void finalizeSYCLDelayedAnalysis(const FunctionDecl *Caller, const FunctionDecl *Callee, - SourceLocation Loc); + SourceLocation Loc, + DeviceDiagnosticReason Reason); /// Tells whether given variable is a SYCL explicit SIMD extension's "private /// global" variable - global variable in the private address space. diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index b388fe1428659..ac9106bbf6a6d 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -1665,7 +1665,7 @@ class DeferredDiagnosticsEmitter S.finalizeOpenMPDelayedAnalysis(Caller, FD, Loc); // Finalize analysis of SYCL-specific constructs. if (Caller && S.LangOpts.SYCLIsDevice) - S.finalizeSYCLDelayedAnalysis(Caller, FD, Loc); + S.finalizeSYCLDelayedAnalysis(Caller, FD, Loc, RootReason); if (Caller) S.DeviceKnownEmittedFns[FD] = {Caller, Loc}; // Always emit deferred diagnostics for the direct users. This does not diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index c6a1e7899ccde..6e5ef912bed15 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18469,11 +18469,20 @@ Decl *Sema::getObjCDeclContext() const { } Sema::DeviceDiagnosticReason Sema::getEmissionReason(const FunctionDecl *FD) { + // FIXME: This should really be a bitwise-or of the language modes. if (FD->hasAttr()) return Sema::DeviceDiagnosticReason::Esimd; - else if (FD->hasAttr() || FD->hasAttr()) + if (FD->hasAttr() || FD->hasAttr()) return Sema::DeviceDiagnosticReason::Sycl; - // FIXME: Figure out the logic for OMP and CUDA. + // FIXME: Refine the logic for CUDA and OpenMP. + if (getLangOpts().CUDA && getLangOpts().CUDAIsDevice) + return Sema::DeviceDiagnosticReason::CudaDevice; + if (getLangOpts().CUDA && !getLangOpts().CUDAIsDevice) + return Sema::DeviceDiagnosticReason::CudaHost; + if (getLangOpts().OpenMPIsDevice) + return Sema::DeviceDiagnosticReason::OmpDevice; + if (getLangOpts().OpenMP && !getLangOpts().OpenMPIsDevice) + return Sema::DeviceDiagnosticReason::OmpHost; return Sema::DeviceDiagnosticReason::All; } diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 7cfb5bdbc5b7a..2a144d7f5cd31 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -597,6 +597,15 @@ class SingleDeviceFunctionTracker { return; } + // If this is a routine that is not defined and it does not have either + // a SYCLKernel or SYCLDevice attribute on it, add it to the set of + // routines potentially reachable on device. This is to diagnose such + // cases later in finalizeSYCLDeviceAnalysis(). + if (!CurrentDecl->isDefined() && !CurrentDecl->hasAttr() && + !CurrentDecl->hasAttr()) + Parent.SemaRef.addFDToReachableFromSyclDevice(CurrentDecl, + CallStack.back()); + // We previously thought we could skip this function if we'd seen it before, // but if we haven't seen it before in this call graph, we can end up // missing a recursive call. SO, we have to revisit call-graphs we've @@ -3885,22 +3894,29 @@ bool Sema::checkSYCLDeviceFunction(SourceLocation Loc, FunctionDecl *Callee) { void Sema::finalizeSYCLDelayedAnalysis(const FunctionDecl *Caller, const FunctionDecl *Callee, - SourceLocation Loc) { + SourceLocation Loc, + DeviceDiagnosticReason Reason) { // Somehow an unspecialized template appears to be in callgraph or list of // device functions. We don't want to emit diagnostic here. if (Callee->getTemplatedKind() == FunctionDecl::TK_FunctionTemplate) return; Callee = Callee->getMostRecentDecl(); - bool HasAttr = - Callee->hasAttr() || Callee->hasAttr(); - // Disallow functions with neither definition nor SYCL_EXTERNAL mark - bool NotDefinedNoAttr = !Callee->isDefined() && !HasAttr; + // If the reason for the emission of this diagnostic is not SYCL-specific, + // and it is not known to be reachable from a routine on device, do not + // issue a diagnostic. + if ((Reason & DeviceDiagnosticReason::Sycl) == DeviceDiagnosticReason::None && + !isFDReachableFromSyclDevice(Callee, Caller)) + return; + + // If Callee has a SYCL attribute, no diagnostic needed. + if (Callee->hasAttr() || Callee->hasAttr()) + return; - if (NotDefinedNoAttr && !Callee->getBuiltinID()) { - Diag(Loc, diag::err_sycl_restrict) - << Sema::KernelCallUndefinedFunction; + // Diagnose if this is an undefined function and it is not a builtin. + if (!Callee->isDefined() && !Callee->getBuiltinID()) { + Diag(Loc, diag::err_sycl_restrict) << Sema::KernelCallUndefinedFunction; Diag(Callee->getLocation(), diag::note_previous_decl) << Callee; Diag(Caller->getLocation(), diag::note_called_by) << Caller; } diff --git a/clang/test/SemaSYCL/call-to-undefined-function.cpp b/clang/test/SemaSYCL/call-to-undefined-function.cpp index 9ab0034c45cfa..12570c5ce1005 100644 --- a/clang/test/SemaSYCL/call-to-undefined-function.cpp +++ b/clang/test/SemaSYCL/call-to-undefined-function.cpp @@ -1,4 +1,8 @@ // RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2020 -verify -fsyntax-only %s +// RUN: %clang_cc1 -fsycl-is-device -fopenmp-simd -internal-isystem %S/Inputs -sycl-std=2020 -verify -fsyntax-only %s + +// This test checks whether we diagnose cases of unmarked, undefined +// functions called on device from either kernels or sycl device functions. #include "sycl.hpp" From a7b227754d344397153580336c12c548341df748 Mon Sep 17 00:00:00 2001 From: Premanand M Rao Date: Thu, 13 May 2021 06:36:48 -0700 Subject: [PATCH 2/4] Fix clang-format errors --- clang/include/clang/Sema/Sema.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 70245d26b86e7..5bc095b405fda 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1819,14 +1819,14 @@ class Sema final { // Helper routine to add a pair of Callee-Caller pair of FunctionDecl * // to UndefinedReachableFromSyclDevice. void addFDToReachableFromSyclDevice(FunctionDecl *Callee, - FunctionDecl* Caller) { + FunctionDecl *Caller) { UndefinedReachableFromSyclDevice.push_back(std::make_pair(Callee, Caller)); } // Helper routine to check if a pair of Callee-Caller FunctionDecl * // is in UndefinedReachableFromSyclDevice. bool isFDReachableFromSyclDevice(const FunctionDecl *Callee, const FunctionDecl *Caller) { - for (auto It: UndefinedReachableFromSyclDevice) + for (auto It : UndefinedReachableFromSyclDevice) if (It.first == Callee && It.second == Caller) return true; return false; From 12b6beb760fbca100b04e4ffad34a8b788006492 Mon Sep 17 00:00:00 2001 From: Premanand M Rao Date: Thu, 13 May 2021 12:06:19 -0700 Subject: [PATCH 3/4] Address code-review comments --- clang/include/clang/Sema/Sema.h | 12 ++++++------ clang/lib/Sema/SemaDecl.cpp | 16 ++++++++-------- clang/lib/Sema/SemaSYCL.cpp | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 5bc095b405fda..35407d6e1def2 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -1818,18 +1818,18 @@ class Sema final { public: // Helper routine to add a pair of Callee-Caller pair of FunctionDecl * // to UndefinedReachableFromSyclDevice. - void addFDToReachableFromSyclDevice(FunctionDecl *Callee, - FunctionDecl *Caller) { + void addFDToReachableFromSyclDevice(const FunctionDecl *Callee, + const FunctionDecl *Caller) { UndefinedReachableFromSyclDevice.push_back(std::make_pair(Callee, Caller)); } // Helper routine to check if a pair of Callee-Caller FunctionDecl * // is in UndefinedReachableFromSyclDevice. bool isFDReachableFromSyclDevice(const FunctionDecl *Callee, const FunctionDecl *Caller) { - for (auto It : UndefinedReachableFromSyclDevice) - if (It.first == Callee && It.second == Caller) - return true; - return false; + return llvm::any_of(UndefinedReachableFromSyclDevice, + [Callee, Caller](const CallPair &P) { + return P.first == Callee && P.second == Caller; + }); } /// A generic diagnostic builder for errors which may or may not be deferred. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 6e5ef912bed15..1dfdf8dc28554 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18475,14 +18475,14 @@ Sema::DeviceDiagnosticReason Sema::getEmissionReason(const FunctionDecl *FD) { if (FD->hasAttr() || FD->hasAttr()) return Sema::DeviceDiagnosticReason::Sycl; // FIXME: Refine the logic for CUDA and OpenMP. - if (getLangOpts().CUDA && getLangOpts().CUDAIsDevice) - return Sema::DeviceDiagnosticReason::CudaDevice; - if (getLangOpts().CUDA && !getLangOpts().CUDAIsDevice) - return Sema::DeviceDiagnosticReason::CudaHost; - if (getLangOpts().OpenMPIsDevice) - return Sema::DeviceDiagnosticReason::OmpDevice; - if (getLangOpts().OpenMP && !getLangOpts().OpenMPIsDevice) - return Sema::DeviceDiagnosticReason::OmpHost; + if (getLangOpts().CUDA) + return getLangOpts().CUDAIsDevice ? + Sema::DeviceDiagnosticReason::CudaDevice : + Sema::DeviceDiagnosticReason::CudaHost; + if (getLangOpts().OpenMP) + return getLangOpts().OpenMPIsDevice ? + Sema::DeviceDiagnosticReason::OmpDevice : + Sema::DeviceDiagnosticReason::OmpHost; return Sema::DeviceDiagnosticReason::All; } diff --git a/clang/lib/Sema/SemaSYCL.cpp b/clang/lib/Sema/SemaSYCL.cpp index 2a144d7f5cd31..9142197e13d5b 100644 --- a/clang/lib/Sema/SemaSYCL.cpp +++ b/clang/lib/Sema/SemaSYCL.cpp @@ -599,7 +599,7 @@ class SingleDeviceFunctionTracker { // If this is a routine that is not defined and it does not have either // a SYCLKernel or SYCLDevice attribute on it, add it to the set of - // routines potentially reachable on device. This is to diagnose such + // routines potentially reachable on device. This is to diagnose such // cases later in finalizeSYCLDeviceAnalysis(). if (!CurrentDecl->isDefined() && !CurrentDecl->hasAttr() && !CurrentDecl->hasAttr()) From acbf79157e174978ab85034487e0f4e9aaa2ea30 Mon Sep 17 00:00:00 2001 From: Premanand M Rao Date: Thu, 13 May 2021 12:28:00 -0700 Subject: [PATCH 4/4] Fix clang-format errors --- clang/lib/Sema/SemaDecl.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 1dfdf8dc28554..64981bc6a9594 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -18476,13 +18476,12 @@ Sema::DeviceDiagnosticReason Sema::getEmissionReason(const FunctionDecl *FD) { return Sema::DeviceDiagnosticReason::Sycl; // FIXME: Refine the logic for CUDA and OpenMP. if (getLangOpts().CUDA) - return getLangOpts().CUDAIsDevice ? - Sema::DeviceDiagnosticReason::CudaDevice : - Sema::DeviceDiagnosticReason::CudaHost; + return getLangOpts().CUDAIsDevice ? Sema::DeviceDiagnosticReason::CudaDevice + : Sema::DeviceDiagnosticReason::CudaHost; if (getLangOpts().OpenMP) - return getLangOpts().OpenMPIsDevice ? - Sema::DeviceDiagnosticReason::OmpDevice : - Sema::DeviceDiagnosticReason::OmpHost; + return getLangOpts().OpenMPIsDevice + ? Sema::DeviceDiagnosticReason::OmpDevice + : Sema::DeviceDiagnosticReason::OmpHost; return Sema::DeviceDiagnosticReason::All; }