From d476f8a347bdb9e56abae0ec58c3ee23f2185ba6 Mon Sep 17 00:00:00 2001 From: Artem Gindinson Date: Wed, 22 Jan 2020 19:53:11 +0300 Subject: [PATCH 1/2] [SYCL][Driver] Improve the diagnostic for FPGA device link errors FPGA compilations have a common use case of linking an object file against an arhive that contains a pre-compiled device image. If the archive was comprised without pulling in the object file in question, the presence of any device kernel in the object file would signal incomplete device code compilation/linkage. Within the SYCL flow, a check for this case is performed by the llvm-no-spir-kernel tool, so as to achieve a compile-time failure instead of an obscure runtime failure due to a missing kernel. This patch extends the Clang driver with the infrastructure to enhance error messages upon receiving specific error codes from external tools. The functionality is then used to enhance the diagnostic yielded by llvm-no-spir-kernel in the aforementioned case. Signed-off-by: Artem Gindinson --- clang/include/clang/Driver/Job.h | 18 +++++++++++++ clang/lib/Driver/Driver.cpp | 6 ++++- clang/lib/Driver/Job.cpp | 11 ++++++++ clang/lib/Driver/ToolChains/Clang.cpp | 23 ++++++++++++----- clang/lib/Driver/ToolChains/Clang.h | 1 + .../test/fpga_tests/fpga_device_link_diag.cpp | 25 +++++++++++++++++++ 6 files changed, 77 insertions(+), 7 deletions(-) create mode 100644 sycl/test/fpga_tests/fpga_device_link_diag.cpp diff --git a/clang/include/clang/Driver/Job.h b/clang/include/clang/Driver/Job.h index 0765b3c67d4e7..5862f40a68c6f 100644 --- a/clang/include/clang/Driver/Job.h +++ b/clang/include/clang/Driver/Job.h @@ -11,6 +11,7 @@ #include "clang/Basic/LLVM.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" @@ -39,6 +40,10 @@ struct CrashReportInfo { /// Command - An executable path/name and argument vector to /// execute. class Command { +public: + using ErrorCodeDiagMapTy = llvm::DenseMap; + +private: /// Source - The action which caused the creation of this job. const Action &Source; @@ -48,6 +53,10 @@ class Command { /// The executable to run. const char *Executable; + /// The container for custom driver-set diagnostic messages that are + /// produced upon particular error codes returned by the command + ErrorCodeDiagMapTy ErrorCodeDiagMap; + /// The list of program arguments (not including the implicit first /// argument, which will be the executable). llvm::opt::ArgStringList Arguments; @@ -100,6 +109,15 @@ class Command { virtual int Execute(ArrayRef> Redirects, std::string *ErrMsg, bool *ExecutionFailed) const; + /// Store a custom driver diagnostic message upon a particular error code + /// returned by the command + void addDiagForErrorCode(int ErrorCode, StringRef CustomDiag); + + /// Get the custom driver diagnostic message for a particular error code + /// if such was stored. Returns an empty string if no diagnostic message + /// was found for the given error code. + StringRef getDiagForErrorCode(int ErrorCode) const; + /// getSource - Return the Action which caused the creation of this job. const Action &getSource() const { return Source; } diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index ce4a8f43c1af0..43897a7735836 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -1686,7 +1686,7 @@ int Driver::ExecuteCompilation( // diagnostics, so always print the diagnostic there. const Tool &FailingTool = FailingCommand->getCreator(); - if (!FailingCommand->getCreator().hasGoodDiagnostics() || CommandRes != 1) { + if (!FailingTool.hasGoodDiagnostics() || CommandRes != 1) { // FIXME: See FIXME above regarding result code interpretation. if (CommandRes < 0) Diag(clang::diag::err_drv_command_signalled) @@ -1695,6 +1695,10 @@ int Driver::ExecuteCompilation( Diag(clang::diag::err_drv_command_failed) << FailingTool.getShortName() << CommandRes; } + + auto CustomDiag = FailingCommand->getDiagForErrorCode(CommandRes); + if (!CustomDiag.empty()) + Diag(clang::diag::note_drv_command_failed_diag_msg) << CustomDiag; } return Res; } diff --git a/clang/lib/Driver/Job.cpp b/clang/lib/Driver/Job.cpp index d57c3a1cdbb89..b31f89bd2f7f9 100644 --- a/clang/lib/Driver/Job.cpp +++ b/clang/lib/Driver/Job.cpp @@ -173,6 +173,17 @@ void Command::buildArgvForResponseFile( } } +void Command::addDiagForErrorCode(int ErrorCode, StringRef CustomDiag) { + ErrorCodeDiagMap[ErrorCode] = CustomDiag.str(); +} + +StringRef Command::getDiagForErrorCode(int ErrorCode) const { + auto ErrorCodeDiagIt = ErrorCodeDiagMap.find(ErrorCode); + if (ErrorCodeDiagIt != ErrorCodeDiagMap.end()) + return ErrorCodeDiagIt->second; + return StringRef(); +} + /// Rewrite relative include-like flag paths to absolute ones. static void rewriteIncludes(const llvm::ArrayRef &Args, size_t Idx, diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp index 9e919c78bdc26..ab4c3b468b960 100644 --- a/clang/lib/Driver/ToolChains/Clang.cpp +++ b/clang/lib/Driver/ToolChains/Clang.cpp @@ -7431,18 +7431,29 @@ void SPIRCheck::ConstructJob(Compilation &C, const JobAction &JA, // we need to exit. The expected output is the input as this is just an // intermediate check with no functional change. ArgStringList CheckArgs; - for (auto I : Inputs) { - CheckArgs.push_back(I.getFilename()); - } + assert(Inputs.size() == 1 && "Unexpected number of inputs to the tool"); + const InputInfo &InputFile = Inputs.front(); + CheckArgs.push_back(InputFile.getFilename()); // Add output file, which is just a copy of the input to better fit in the // toolchain flow. CheckArgs.push_back("-o"); CheckArgs.push_back(Output.getFilename()); - - C.addCommand(std::make_unique(JA, *this, + auto Cmd = std::make_unique( + JA, *this, TCArgs.MakeArgString(getToolChain().GetProgramPath(getShortName())), - CheckArgs, None)); + CheckArgs, None); + + if (getToolChain().getTriple().getSubArch() == + llvm::Triple::SPIRSubArch_fpga) { + const char *Msg = TCArgs.MakeArgString( + Twine("The FPGA image does not include all device kernels from ") + + Twine(InputFile.getBaseInput()) + + Twine(". Please re-generate the image")); + Cmd->addDiagForErrorCode(/*ErrorCode*/ 1, Msg); + } + + C.addCommand(std::move(Cmd)); } void SYCLPostLink::ConstructJob(Compilation &C, const JobAction &JA, diff --git a/clang/lib/Driver/ToolChains/Clang.h b/clang/lib/Driver/ToolChains/Clang.h index a7dec20d8dbfa..0cd0e16ad7784 100644 --- a/clang/lib/Driver/ToolChains/Clang.h +++ b/clang/lib/Driver/ToolChains/Clang.h @@ -199,6 +199,7 @@ class LLVM_LIBRARY_VISIBILITY SYCLPostLink final : public Tool { : Tool("SYCL post link", "sycl-post-link", TC) {} bool hasIntegratedCPP() const override { return false; } + bool hasGoodDiagnostics() const override { return true; } void ConstructJob(Compilation &C, const JobAction &JA, const InputInfo &Output, const InputInfoList &Inputs, const llvm::opt::ArgList &TCArgs, diff --git a/sycl/test/fpga_tests/fpga_device_link_diag.cpp b/sycl/test/fpga_tests/fpga_device_link_diag.cpp new file mode 100644 index 0000000000000..3eadd9872cd9b --- /dev/null +++ b/sycl/test/fpga_tests/fpga_device_link_diag.cpp @@ -0,0 +1,25 @@ +//==----- fpga_device_link_diag.cpp - SYCL FPGA linking diagnostic test ----==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// REQUIRES: aoc, accelerator + +// RUN: %clangxx -fsycl -fintelfpga %s -c -o test_obj.o +// RUN: touch dummy.cpp +// RUN: %clangxx -fsycl -fintelfpga dummy.cpp -c +// RUN: %clangxx -fsycl -fintelfpga -fsycl-link=image dummy.o -o dummy_arch.a +// RUN: not %clangxx -fsycl -fintelfpga test_obj.o dummy_arch.a 2>&1 | FileCheck %s --check-prefix=CHK-FPGA-LINK-DIAG +// CHK-FPGA-LINK-DIAG: note: diagnostic msg: The FPGA image does not include all device kernels from test_obj.o. Please re-generate the image + +template +__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { + kernelFunc(); +} + +void foo() { + kernel_single_task([]() {}); +} From 7ae9874c31bf28523724474c554be9d267ffa6a2 Mon Sep 17 00:00:00 2001 From: Artem Gindinson Date: Wed, 5 Feb 2020 13:48:07 +0300 Subject: [PATCH 2/2] Document the error-code handling infrastructure specifics Signed-off-by: Artem Gindinson --- clang/include/clang/Driver/Job.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Driver/Job.h b/clang/include/clang/Driver/Job.h index 5862f40a68c6f..18f02b6cdd2b3 100644 --- a/clang/include/clang/Driver/Job.h +++ b/clang/include/clang/Driver/Job.h @@ -54,7 +54,15 @@ class Command { const char *Executable; /// The container for custom driver-set diagnostic messages that are - /// produced upon particular error codes returned by the command + /// produced upon particular error codes returned by the command. + /// In order to add such a diagnostic for an external tool, consider the + /// following criteria: + /// 1) Does the command's executable return different codes upon different + /// types of errors? + /// 2) If the executable provides a single error code for various error types, + /// is only a certain type of failure expected to occur within the driver + /// flow? E.g. the driver guarantees a valid input to the tool, so any + /// "invalid input" error can be ruled out ErrorCodeDiagMapTy ErrorCodeDiagMap; /// The list of program arguments (not including the implicit first