From 112d238ff2fcfb45c133c6602f85beac7eef73a9 Mon Sep 17 00:00:00 2001 From: Andrew Gozillon Date: Tue, 28 May 2019 19:37:04 -0700 Subject: [PATCH 1/4] [Driver][Offloader] Add getOffloadingDeviceToolChain function getOffloadingDeviceToolChain is the equivalent of getToolChain for device ToolChains. It may be possible to merge the two but as Offloading Devices require some extra information (like host/device triple and OffloadKind) it may get a little messy. The switch statement in getToolChain also already seems a little complex. So it may not be worth merging and over complicating them. In general using getOffloadingDeviceToolChain moves some of the shared logic for device ToolChain selection for SYCL/CUDA/HIP/OpenMP into the function call, so it refactors it a bit. But in a more specific application to the SYCL implementation this call allows a ToolChain implementer to add their own ToolChain in place of the SYCL ToolChain when targeting a specific host / device triple. However, if they did not want to add their own ToolChain and simply wanted to add a new tool to the existing SYCL ToolChain they'd still be able to do that via overloading/adding to the GetTools method inside of the SYCLToolChain. So the intent is just to add more flexibility, rather than replace some functionality with some other functionality. Signed-off-by: Andrew Gozillon --- clang/include/clang/Driver/Driver.h | 16 ++++ clang/lib/Driver/Driver.cpp | 110 ++++++++++++++++++---------- 2 files changed, 86 insertions(+), 40 deletions(-) diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h index 03e6458a5e5d5..72604bb60b978 100644 --- a/clang/include/clang/Driver/Driver.h +++ b/clang/include/clang/Driver/Driver.h @@ -557,6 +557,22 @@ class Driver { /// @} + /// Retrieves a ToolChain for a particular device \p Target triple + /// + /// \param[in] HostTC is the host ToolChain paired with the device + /// + /// \param[in] Action (e.g. OFK_Cuda/OFK_OpenMP/OFK_SYCL) is an Offloading + /// action that is optionally passed to a ToolChain (used by CUDA, to specify + /// if it's used in conjunction with OpenMP) + /// + /// Will cache ToolChains for the life of the driver object, and create them + /// on-demand. + const ToolChain &getOffloadingDeviceToolChain(const llvm::opt::ArgList &Args, + const llvm::Triple &Target, + const ToolChain &HostTC, + const Action::OffloadKind + &TargetDeviceOffloadKind) const; + /// Get bitmasks for which option flags to include and exclude based on /// the driver mode. std::pair getIncludeExcludeOptionFlagMasks(bool IsClCompatMode) const; diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index cb4306d3fc4d6..586e57568b47f 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -621,14 +621,12 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, DeviceTripleStr = HostTriple.isArch64Bit() ? "nvptx64-nvidia-cuda" : "nvptx-nvidia-cuda"; llvm::Triple CudaTriple(DeviceTripleStr); - // Use the CUDA and host triples as the key into the ToolChains map, - // because the device toolchain we create depends on both. - auto &CudaTC = ToolChains[CudaTriple.str() + "/" + HostTriple.str()]; - if (!CudaTC) { - CudaTC = llvm::make_unique( - *this, CudaTriple, *HostTC, C.getInputArgs(), OFK); - } - C.addOffloadDeviceToolChain(CudaTC.get(), OFK); + // Use the CUDA and host triples as the key into the + // getOffloadingDeviceToolChain, because the device toolchain we + // create depends on both. + auto CudaTC = &getOffloadingDeviceToolChain(C.getInputArgs(), CudaTriple, + *HostTC, OFK); + C.addOffloadDeviceToolChain(CudaTC, OFK); } else if (IsHIP) { const ToolChain *HostTC = C.getSingleOffloadToolChain(); const llvm::Triple &HostTriple = HostTC->getTriple(); @@ -636,14 +634,12 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, auto OFK = Action::OFK_HIP; DeviceTripleStr = "amdgcn-amd-amdhsa"; llvm::Triple HIPTriple(DeviceTripleStr); - // Use the HIP and host triples as the key into the ToolChains map, - // because the device toolchain we create depends on both. - auto &HIPTC = ToolChains[HIPTriple.str() + "/" + HostTriple.str()]; - if (!HIPTC) { - HIPTC = llvm::make_unique( - *this, HIPTriple, *HostTC, C.getInputArgs()); - } - C.addOffloadDeviceToolChain(HIPTC.get(), OFK); + // Use the HIP and host triples as the key into + // getOffloadingDeviceToolChain, because the device toolchain we create + // depends on both. + auto HIPTC = &getOffloadingDeviceToolChain(C.getInputArgs(), HIPTriple, + *HostTC, OFK); + C.addOffloadDeviceToolChain(HIPTC, OFK); } // @@ -695,12 +691,8 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, const ToolChain *HostTC = C.getSingleOffloadToolChain(); assert(HostTC && "Host toolchain should be always defined."); - auto &CudaTC = - ToolChains[TT.str() + "/" + HostTC->getTriple().normalize()]; - if (!CudaTC) - CudaTC = llvm::make_unique( - *this, TT, *HostTC, C.getInputArgs(), Action::OFK_OpenMP); - TC = CudaTC.get(); + TC = &getOffloadingDeviceToolChain(C.getInputArgs(), TT, *HostTC, + Action::OFK_OpenMP); } else TC = &getToolChain(C.getInputArgs(), TT); C.addOffloadDeviceToolChain(TC, Action::OFK_OpenMP); @@ -781,15 +773,13 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, else { const ToolChain *HostTC = C.getSingleOffloadToolChain(); - const llvm::Triple &HostTriple = HostTC->getTriple(); - // Use the SYCL and host triples as the key into the ToolChains map, - // because the device toolchain we create depends on both. - auto &SYCLTC = ToolChains[TT.str() + "/" + HostTriple.str()]; - if (!SYCLTC) { - SYCLTC = llvm::make_unique( - *this, TT, *HostTC, C.getInputArgs()); - } - C.addOffloadDeviceToolChain(SYCLTC.get(), Action::OFK_SYCL); + // Use the SYCL and host triples as the key into + // getOffloadingDeviceToolChain, because the device toolchain we + // create depends on both. + auto SYCLTC = &getOffloadingDeviceToolChain(C.getInputArgs(), TT, + *HostTC, + Action::OFK_SYCL); + C.addOffloadDeviceToolChain(SYCLTC, Action::OFK_SYCL); } } } else { @@ -811,15 +801,12 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, TT.setVendor(llvm::Triple::UnknownVendor); TT.setOS(llvm::Triple(llvm::sys::getProcessTriple()).getOS()); TT.setEnvironment(llvm::Triple::SYCLDevice); - // Use the SYCL and host triples as the key into the ToolChains map, - // because the device toolchain we create depends on both. - auto &SYCLTC = ToolChains[(TT.normalize() + Twine("/") + - HostTriple.normalize()).str()]; - if (!SYCLTC) { - SYCLTC = llvm::make_unique( - *this, TT, *HostTC, C.getInputArgs()); - } - C.addOffloadDeviceToolChain(SYCLTC.get(), Action::OFK_SYCL); + // Use the SYCL and host triples as the key into + // getOffloadingDeviceToolChain, because the device toolchain we create + // depends on both. + auto SYCLTC = &getOffloadingDeviceToolChain(C.getInputArgs(), TT, *HostTC, + Action::OFK_SYCL); + C.addOffloadDeviceToolChain(SYCLTC, Action::OFK_SYCL); } } @@ -5134,6 +5121,49 @@ const ToolChain &Driver::getToolChain(const ArgList &Args, return *TC; } +const ToolChain &Driver::getOffloadingDeviceToolChain(const ArgList &Args, + const llvm::Triple &Target, const ToolChain &HostTC, + const Action::OffloadKind &TargetDeviceOffloadKind) const { + // Use device / host triples as the key into the ToolChains map because the + // device ToolChain we create depends on both. + auto &TC = ToolChains[Target.str() + "/" + HostTC.getTriple().str()]; + if (!TC) { + // Categorized by offload kind > arch rather than OS > arch like + // the normal getToolChain call, as it seems a reasonable way to categorize + // things. + switch (TargetDeviceOffloadKind) { + case Action::OFK_Cuda: + TC = llvm::make_unique( + *this, Target, HostTC, Args, TargetDeviceOffloadKind); + break; + case Action::OFK_HIP: + TC = llvm::make_unique( + *this, Target, HostTC, Args); + break; + case Action::OFK_OpenMP: + // omp + nvptx + TC = llvm::make_unique( + *this, Target, HostTC, Args, TargetDeviceOffloadKind); + break; + case Action::OFK_SYCL: + switch (Target.getArch()) { + case llvm::Triple::spir: + case llvm::Triple::spir64: + TC = llvm::make_unique( + *this, Target, HostTC, Args); + break; + default: + llvm_unreachable("Unexpected option."); + } + break; + default: + llvm_unreachable("Unexpected option."); + } + } + + return *TC; +} + bool Driver::ShouldUseClangCompiler(const JobAction &JA) const { // Say "no" if there is not exactly one input of a type clang understands. if (JA.size() != 1 || From fb50177ec5e1d9df1f4873fe49b66b461a4c398b Mon Sep 17 00:00:00 2001 From: Andrew Gozillon Date: Wed, 29 May 2019 13:27:10 -0700 Subject: [PATCH 2/4] [SYCL][Driver][Offloader] Adding appropriate error out on incorrect SYCL target No longer will the switch statement assert with llvm_unreachable, instead the appropriate Clang diagnostic warning for an invalid SYCL output should be emitted. This occurs in the initialize() call for the SYCL offloader/phase and action generator, similar to how CUDA emits errors for incorrect GPU Arch targets. Doing it similarly to OpenMP (inside CompilerInvocation.cpp) doesn't work in this case as if we pass an invalid target we won't accidentally generate an invalid/incorrect ToolChain like GetToolChain's we will instead return a nullptr which will ICE the compiler before it reaches CompilerInvocation.cpp, so the warning interaction has to be slightly different in this case. Also removing some unused variables I missed in the first commit. Signed-off-by: Andrew Gozillon --- clang/lib/Driver/Driver.cpp | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index 586e57568b47f..0d69256c2ecfe 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -629,7 +629,6 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, C.addOffloadDeviceToolChain(CudaTC, OFK); } else if (IsHIP) { const ToolChain *HostTC = C.getSingleOffloadToolChain(); - const llvm::Triple &HostTriple = HostTC->getTriple(); StringRef DeviceTripleStr; auto OFK = Action::OFK_HIP; DeviceTripleStr = "amdgcn-amd-amdhsa"; @@ -795,7 +794,6 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, if (HasValidSYCLRuntime) { const ToolChain *HostTC = C.getSingleOffloadToolChain(); - const llvm::Triple &HostTriple = HostTC->getTriple(); llvm::Triple TT(TargetTriple); TT.setArch(llvm::Triple::spir64); TT.setVendor(llvm::Triple::UnknownVendor); @@ -3205,6 +3203,23 @@ class OffloadingActionBuilder final { } bool initialize() override { + // Get and check the SYCL target triples if any, if it's not a valid + // triple print a diagnostic and return true stating the initialization + // has failed + if (Arg *A = Args.getLastArg(options::OPT_fsycl_targets_EQ)) { + for (unsigned i = 0; i < A->getNumValues(); ++i) { + llvm::Triple TT(A->getValue(i)); + + if (TT.getArch() == llvm::Triple::UnknownArch || + !(TT.getArch() == llvm::Triple::spir || + TT.getArch() == llvm::Triple::spir64)) { + C.getDriver().Diag(diag::err_drv_invalid_sycl_target) + << A->getValue(i); + return true; + } + } + } + // Get the SYCL toolchains. If we don't get any, the action builder will // know there is nothing to do related to SYCL offloading. auto SYCLTCRange = C.getOffloadToolChains(); @@ -5153,11 +5168,11 @@ const ToolChain &Driver::getOffloadingDeviceToolChain(const ArgList &Args, *this, Target, HostTC, Args); break; default: - llvm_unreachable("Unexpected option."); + break; } break; default: - llvm_unreachable("Unexpected option."); + break; } } From 767cc694dd5ee1b82684c182cf3a97b4c64a862c Mon Sep 17 00:00:00 2001 From: Andrew Gozillon Date: Thu, 30 May 2019 15:11:28 -0700 Subject: [PATCH 3/4] [SYCL][Driver][Offloader][NFC] Moving invalid SYCL target diagnostic This should be an NFC. Moved the diagnostic from the OffloadingActionBuilder's initialize method to the Driver's CreateOffloadingDeviceToolChains call. No need to track it in two places, invalid target caught as early as required. Should be no chance of ICE'ng the compiler by accessing a non-existant ToolChain. Signed-off-by: Andrew Gozillon --- clang/lib/Driver/Driver.cpp | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index 0d69256c2ecfe..3bd805ff45e0b 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -767,7 +767,9 @@ void Driver::CreateOffloadingDeviceToolChains(Compilation &C, FoundNormalizedTriples[NormalizedName] = Val; // If the specified target is invalid, emit a diagnostic. - if (TT.getArch() == llvm::Triple::UnknownArch) + if (TT.getArch() == llvm::Triple::UnknownArch || + !(TT.getArch() == llvm::Triple::spir || + TT.getArch() == llvm::Triple::spir64)) Diag(clang::diag::err_drv_invalid_sycl_target) << Val; else { const ToolChain *HostTC = @@ -3203,23 +3205,6 @@ class OffloadingActionBuilder final { } bool initialize() override { - // Get and check the SYCL target triples if any, if it's not a valid - // triple print a diagnostic and return true stating the initialization - // has failed - if (Arg *A = Args.getLastArg(options::OPT_fsycl_targets_EQ)) { - for (unsigned i = 0; i < A->getNumValues(); ++i) { - llvm::Triple TT(A->getValue(i)); - - if (TT.getArch() == llvm::Triple::UnknownArch || - !(TT.getArch() == llvm::Triple::spir || - TT.getArch() == llvm::Triple::spir64)) { - C.getDriver().Diag(diag::err_drv_invalid_sycl_target) - << A->getValue(i); - return true; - } - } - } - // Get the SYCL toolchains. If we don't get any, the action builder will // know there is nothing to do related to SYCL offloading. auto SYCLTCRange = C.getOffloadToolChains(); From 78272d554ce4f5677bccd9232ea354c693b6bd4d Mon Sep 17 00:00:00 2001 From: Andrew Gozillon Date: Fri, 31 May 2019 13:51:37 -0700 Subject: [PATCH 4/4] [SYCL][Driver]Offloader][Test] Add test case to sycl-offload.c New test case aims to showcase that existing architecture triples that are not valid for SYCL offloading also get rejected. In this case, just testing using x86_64. Signed-off-by: Andrew Gozillon --- clang/test/Driver/sycl-offload.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/clang/test/Driver/sycl-offload.c b/clang/test/Driver/sycl-offload.c index df9b2d47910e3..66568d7ca8925 100644 --- a/clang/test/Driver/sycl-offload.c +++ b/clang/test/Driver/sycl-offload.c @@ -14,6 +14,13 @@ /// ########################################################################### +/// Check whether an invalid SYCL target is specified: +// RUN: %clang -### -fsycl -fsycl-targets=x86_64 %s 2>&1 \ +// RUN: | FileCheck -check-prefix=CHK-INVALID-REAL-TARGET %s +// CHK-INVALID-REAL-TARGET: error: SYCL target is invalid: 'x86_64' + +/// ########################################################################### + /// Check warning for empty -fsycl-targets // RUN: %clang -### -fsycl -fsycl-targets= %s 2>&1 \ // RUN: | FileCheck -check-prefix=CHK-EMPTY-SYCLTARGETS %s