Remove Linux search paths on Windows #113628
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
david-salinas
force-pushed
the
rocm_detect_windows
branch
6 times, most recently
from
307e0ab to
2ab5bea
Compare
david-salinas
force-pushed
the
rocm_detect_windows
branch
2 times, most recently
from
4fad2d6 to
a5a8499
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang Author: David Salinas (david-salinas) ChangesChange-Id: Ia0b44eb1069fa631a6d5156cf5881c978e23b62d Full diff: https://github.com/llvm/llvm-project/pull/113628.diff 11 Files Affected:
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 9878a9dad78d40..19f465b7dab08a 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -6440,7 +6440,8 @@ const ToolChain &Driver::getToolChain(const ArgList &Args,
TC = std::make_unique<toolchains::NVPTXToolChain>(*this, Target, Args);
break;
case llvm::Triple::AMDHSA:
- TC = std::make_unique<toolchains::ROCMToolChain>(*this, Target, Args);
+ TC = std::make_unique<toolchains::ROCMToolChain>(*this, Target, Args,
+ Target.isOSWindows());
break;
case llvm::Triple::AMDPAL:
case llvm::Triple::Mesa3D:
diff --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp b/clang/lib/Driver/ToolChains/AMDGPU.cpp
index 2c85d21ebd738c..8a746c3b1359b4 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -306,15 +306,17 @@ RocmInstallationDetector::getInstallationPathCandidates() {
LatestVer = Ver;
}
}
- if (!LatestROCm.empty())
- ROCmSearchDirs.emplace_back(D.SysRoot + "/opt/" + LatestROCm,
- /*StrictChecking=*/true);
- ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
- /*StrictChecking=*/true);
- ROCmSearchDirs.emplace_back(D.SysRoot + "/usr",
- /*StrictChecking=*/true);
+ if (!isHostWindows()) {
+ if (!LatestROCm.empty())
+ ROCmSearchDirs.emplace_back(D.SysRoot + "/opt/" + LatestROCm,
+ /*StrictChecking=*/true);
+ ROCmSearchDirs.emplace_back(D.SysRoot + "/usr/local",
+ /*StrictChecking=*/true);
+ ROCmSearchDirs.emplace_back(D.SysRoot + "/usr",
+ /*StrictChecking=*/true);
+ }
DoPrintROCmSearchDirs();
return ROCmSearchDirs;
}
@@ -375,11 +377,6 @@ RocmInstallationDetector::RocmInstallationDetector(
Twine(DefaultVersionMinor) + "." + VersionPatch)
.str();
}
-
- if (DetectHIPRuntime)
- detectHIPRuntime();
- if (DetectDeviceLib)
- detectDeviceLibrary();
}
void RocmInstallationDetector::detectDeviceLibrary() {
@@ -699,10 +696,12 @@ void amdgpu::getAMDGPUTargetFeatures(const Driver &D,
/// AMDGPU Toolchain
AMDGPUToolChain::AMDGPUToolChain(const Driver &D, const llvm::Triple &Triple,
- const ArgList &Args)
+ const ArgList &Args, bool isHostTCMSVC)
: Generic_ELF(D, Triple, Args),
OptionsDefault(
{{options::OPT_O, "3"}, {options::OPT_cl_std_EQ, "CL1.2"}}) {
+ if (!isHostTCMSVC)
+ RocmInstallation->init();
// Check code object version options. Emit warnings for legacy options
// and errors for the last invalid code object version options.
// It is done here to avoid repeated warning or error messages for
@@ -835,8 +834,11 @@ bool AMDGPUToolChain::isWave64(const llvm::opt::ArgList &DriverArgs,
/// ROCM Toolchain
ROCMToolChain::ROCMToolChain(const Driver &D, const llvm::Triple &Triple,
- const ArgList &Args)
- : AMDGPUToolChain(D, Triple, Args) {
+ const ArgList &Args, bool isHostTCMSVC)
+ : AMDGPUToolChain(D, Triple, Args, isHostTCMSVC) {
+ RocmInstallation->setHostWindows(isHostTCMSVC);
+ if (isHostTCMSVC)
+ RocmInstallation->init(true, false);
RocmInstallation->detectDeviceLibrary();
}
diff --git a/clang/lib/Driver/ToolChains/AMDGPU.h b/clang/lib/Driver/ToolChains/AMDGPU.h
index a9b4552a1f91a4..c0e9727ebf56a2 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.h
+++ b/clang/lib/Driver/ToolChains/AMDGPU.h
@@ -59,7 +59,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF {
public:
AMDGPUToolChain(const Driver &D, const llvm::Triple &Triple,
- const llvm::opt::ArgList &Args);
+ const llvm::opt::ArgList &Args, bool isHostTCMSVC = false);
unsigned GetDefaultDwarfVersion() const override { return 5; }
bool IsMathErrnoDefault() const override { return false; }
@@ -135,7 +135,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF {
class LLVM_LIBRARY_VISIBILITY ROCMToolChain : public AMDGPUToolChain {
public:
ROCMToolChain(const Driver &D, const llvm::Triple &Triple,
- const llvm::opt::ArgList &Args);
+ const llvm::opt::ArgList &Args, bool isHostTCMSVC);
void
addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
diff --git a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
index 3f0b3f2d86b3ed..33462b49196725 100644
--- a/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp
@@ -33,7 +33,8 @@ AMDGPUOpenMPToolChain::AMDGPUOpenMPToolChain(const Driver &D,
const llvm::Triple &Triple,
const ToolChain &HostTC,
const ArgList &Args)
- : ROCMToolChain(D, Triple, Args), HostTC(HostTC) {
+ : ROCMToolChain(D, Triple, Args, HostTC.getTriple().isOSWindows()),
+ HostTC(HostTC) {
// Lookup binaries into the driver directory, this is used to
// discover the 'amdgpu-arch' executable.
getProgramPaths().push_back(getDriver().Dir);
diff --git a/clang/lib/Driver/ToolChains/Gnu.cpp b/clang/lib/Driver/ToolChains/Gnu.cpp
index 8397f1121ec2ce..8e489d3436b108 100644
--- a/clang/lib/Driver/ToolChains/Gnu.cpp
+++ b/clang/lib/Driver/ToolChains/Gnu.cpp
@@ -3060,6 +3060,9 @@ Generic_GCC::Generic_GCC(const Driver &D, const llvm::Triple &Triple,
: ToolChain(D, Triple, Args), GCCInstallation(D),
CudaInstallation(D, Triple, Args), RocmInstallation(D, Triple, Args) {
getProgramPaths().push_back(getDriver().Dir);
+ llvm::Triple TargetTriple(D.getTargetTriple());
+ if (!TargetTriple.isOSWindows())
+ RocmInstallation->init();
}
Generic_GCC::~Generic_GCC() {}
diff --git a/clang/lib/Driver/ToolChains/HIPAMD.cpp b/clang/lib/Driver/ToolChains/HIPAMD.cpp
index bae05cc0bb7353..5a14c96b8484a1 100644
--- a/clang/lib/Driver/ToolChains/HIPAMD.cpp
+++ b/clang/lib/Driver/ToolChains/HIPAMD.cpp
@@ -213,7 +213,14 @@ void AMDGCN::Linker::ConstructJob(Compilation &C, const JobAction &JA,
HIPAMDToolChain::HIPAMDToolChain(const Driver &D, const llvm::Triple &Triple,
const ToolChain &HostTC, const ArgList &Args)
- : ROCMToolChain(D, Triple, Args), HostTC(HostTC) {
+ : ROCMToolChain(D, Triple, Args, HostTC.getTriple().isOSWindows()),
+ HostTC(HostTC) {
+ if (HostTC.getTriple().isWindowsMSVCEnvironment()) {
+ RocmInstallation->setHostWindows(true);
+ }
+
+ RocmInstallation->init(true, false);
+
// Lookup binaries into the driver directory, this is used to
// discover the clang-offload-bundler executable.
getProgramPaths().push_back(getDriver().Dir);
diff --git a/clang/lib/Driver/ToolChains/MSVC.cpp b/clang/lib/Driver/ToolChains/MSVC.cpp
index 80799d1e715f07..ba1130e650b975 100644
--- a/clang/lib/Driver/ToolChains/MSVC.cpp
+++ b/clang/lib/Driver/ToolChains/MSVC.cpp
@@ -425,6 +425,12 @@ MSVCToolChain::MSVCToolChain(const Driver &D, const llvm::Triple &Triple,
const ArgList &Args)
: ToolChain(D, Triple, Args), CudaInstallation(D, Triple, Args),
RocmInstallation(D, Triple, Args) {
+
+ // Tell the ROCm installation detector that Host is Windows before trying to
+ // find HIPRT or Device Libs
+ RocmInstallation->setHostWindows(true);
+ RocmInstallation->init();
+
getProgramPaths().push_back(getDriver().Dir);
std::optional<llvm::StringRef> VCToolsDir, VCToolsVersion;
diff --git a/clang/lib/Driver/ToolChains/ROCm.h b/clang/lib/Driver/ToolChains/ROCm.h
index dceb0ab0366933..a851ecc30d0f43 100644
--- a/clang/lib/Driver/ToolChains/ROCm.h
+++ b/clang/lib/Driver/ToolChains/ROCm.h
@@ -80,6 +80,7 @@ class RocmInstallationDetector {
bool HasHIPStdParLibrary = false;
bool HasRocThrustLibrary = false;
bool HasRocPrimLibrary = false;
+ bool IsHostMSVC = false;
// Default version if not detected or specified.
const unsigned DefaultVersionMajor = 3;
@@ -193,6 +194,10 @@ class RocmInstallationDetector {
/// Check whether we detected a valid HIP STDPAR Acceleration library.
bool hasHIPStdParLibrary() const { return HasHIPStdParLibrary; }
+ /// Check whether the target triple is for Windows.
+ bool isHostWindows() const { return IsHostMSVC; }
+ void setHostWindows(bool val) { IsHostMSVC = val; }
+
/// Print information about the detected ROCm installation.
void print(raw_ostream &OS) const;
@@ -272,6 +277,13 @@ class RocmInstallationDetector {
return Loc->second;
}
+ void init(bool DetectHIPRuntime = true, bool DetectDeviceLib = false) {
+ if (DetectHIPRuntime)
+ detectHIPRuntime();
+ if (DetectDeviceLib)
+ detectDeviceLibrary();
+ }
+
void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args) const;
diff --git a/clang/test/Driver/rocm-detect-linux.hip b/clang/test/Driver/rocm-detect-linux.hip
new file mode 100644
index 00000000000000..26c10c488d9e4d
--- /dev/null
+++ b/clang/test/Driver/rocm-detect-linux.hip
@@ -0,0 +1,9 @@
+// REQUIRES: system-linux
+
+// Test to ensure that on Windows, we do not include linux sesrch paths
+// RUN: %clang -### -nogpulib -nogpuinc \
+// RUN: --print-rocm-search-dirs %s 2>&1 \
+// RUN: | FileCheck %s
+
+// CHECK: ROCm installation search path: {{/usr/local}}
+// CHECK: ROCm installation search path: {{/usr}}
diff --git a/clang/test/Driver/rocm-detect-windows.hip b/clang/test/Driver/rocm-detect-windows.hip
new file mode 100644
index 00000000000000..6642a78a3492f2
--- /dev/null
+++ b/clang/test/Driver/rocm-detect-windows.hip
@@ -0,0 +1,9 @@
+// REQUIRES: system-windows
+
+// Test to ensure that on Windows, we do not include linux sesrch paths
+// RUN: %clang -### -nogpulib -nogpuinc \
+// RUN: --print-rocm-search-dirs %s 2>&1 \
+// RUN: | FileCheck %s
+
+// CHECK-NOT: ROCm installation search path: {{/usr/local}}
+// CHECK-NOT: ROCm installation search path: {{/usr}}
diff --git a/clang/test/Driver/rocm-detect.hip b/clang/test/Driver/rocm-detect.hip
index 4aafeb97c00b5f..6f03c7690a9867 100644
--- a/clang/test/Driver/rocm-detect.hip
+++ b/clang/test/Driver/rocm-detect.hip
@@ -103,7 +103,7 @@
// RUN: %t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin/clang -### -no-canonical-prefixes -v \
// RUN: -resource-dir=%t/rocm-spack/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/lib/clang \
// RUN: -target x86_64-linux-gnu --cuda-gpu-arch=gfx900 --print-rocm-search-dirs %s 2>&1 \
-// RUN: | FileCheck -check-prefixes=SPACK %s
+// RUN: | FileCheck -check-prefixes=SPACK %s --dump-input always --dump-input-context 100
// Test SPACK installation with multiple hip and rocm-device-libs packages of the same
// ROCm release. --hip-path and --rocm-device-lib-path can be used to specify them.
@@ -147,12 +147,12 @@
// ROCM-REL: ROCm installation search path: {{.*}}/opt/rocm-3.10.0
// SPACK: InstalledDir: [[DIR:.*]]/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/bin
+// SPACK: Found HIP installation: [[DIR]]/hip-4.0.0-5f63slrursbrvfe2txrrjkynbsywsob5, version 4.0.20214-a2917cd
// SPACK: ROCm installation search path (Spack 4.0.0): [[DIR]]
// SPACK: ROCm installation search path: [[CLANG:.*]]
// SPACK: ROCm installation search path: [[DIR]]/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z
// SPACK: ROCm installation search path: [[DIR]]/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/lib/clang
// SPACK: ROCm installation search path: /opt/rocm
-// SPACK: Found HIP installation: [[DIR]]/hip-4.0.0-5f63slrursbrvfe2txrrjkynbsywsob5, version 4.0.20214-a2917cd
// SPACK: "-triple" "amdgcn-amd-amdhsa"
// SPACK-SAME: "-mlink-builtin-bitcode" "[[DIR]]/llvm-amdgpu-4.0.0-ieagcs7inf7runpyfvepqkurasoglq4z/amdgcn/bitcode/hip.bc"
// SPACK-SAME: "-idirafter" "[[DIR]]/hip-4.0.0-5f63slrursbrvfe2txrrjkynbsywsob5/include"
|
|
Commit title should be more specific and remove the Gerrit id |
| @@ -0,0 +1,9 @@ | |||
| // REQUIRES: system-windows | |||
There was a problem hiding this comment.
This doesn't require windows, use an explicit triple
There was a problem hiding this comment.
ok, I was just basing this off of existing tests. But I see that recently there has been support for targets in the REQUIRES statements. I'll change these for both the Windows and Linux tests. Also, for the .cl version of the test, I'm not sure if it would change much in how we create and use the ToolChains in the Driver. But, better to be safe. I'll add .cl versions for both tests.
There was a problem hiding this comment.
I don't need .cl, as in OpenCL. I mean cl as in clang-cl, the MSVC compatible driver
|
|
||
| // Tell the ROCm installation detector that Host is Windows before trying to | ||
| // find HIPRT or Device Libs | ||
| RocmInstallation->setHostWindows(true); |
There was a problem hiding this comment.
Toolchain constructor should not imply host
| getProgramPaths().push_back(getDriver().Dir); | ||
| llvm::Triple TargetTriple(D.getTargetTriple()); | ||
| if (!TargetTriple.isOSWindows()) | ||
| RocmInstallation->init(); |
There was a problem hiding this comment.
The RocmInstallation should handle Windows, even if it fails, rather than just masking it here and elsewhere.
| TC = std::make_unique<toolchains::ROCMToolChain>(*this, Target, Args, | ||
| Target.isOSWindows()); |
There was a problem hiding this comment.
Why do we need to pass something implied by the triple when we already have the triple? Also, this is for directly targeting via --target=amdgcn-amd-amdhsa, which isn't Windows.
There was a problem hiding this comment.
The reason we need to pass this is primarily/solely because when we create the Offloading Device ToolChain (in "Driver::getOffloadingDeviceToolChain()", we pass the Device's triple to the toolchain constructor as the target triple; which as you mentioned will not be Windows. Then the Device side toolchain, has it's own RocmInstallationDetector, which will add the linux paths. And I need a way to tell the RocmInstallationDetector, on the device side ToolChain, that the Host TC is actually Windows.
I tried a couple of different approaches to handle this case, but this is the least intrusive so far. The RocmInstallationDetector sets the search paths in its constructor, and all of the ToolChains that have a Rocm Detector call its constructor in their respective initialize list in their constructors. I need a way to tell that detector instance (for a device side toolchain) that it's Host ToolChain is windows and to not add the linux search paths.
There was a problem hiding this comment.
I need to wrap my head around this more. We create multiple toolchains for offloading, the host toolchain and then the additional ones for offloading. All of those toolchains inherit from Generic_GCC in this case which initializes the detector. For the host toolchain this will just be the normal host triple, but for the GPU it should be the GPU target. Since we have the triple in the installation detector, we should just be able to change behavior if the OS is windows? We have the triple argument for ROCm we just don't use it, what's stopping you from just saving that triple inside the class and using it when we do our detection?
There was a problem hiding this comment.
"I need to wrap my head around this more"
Yup, the class hierarchy we have created for our tool chains is ... frustrating, for this case. And I've tried a few different variations just to be able to tell the Device's ToolChain's RocmInstallationDetector to hold off on adding these search paths.
"what's stopping you from just saving that triple inside the class and using it when we do our detection"
That was one of my attempts. I first tried just adding it to the RocmInstallationDetector (RID) class. But because we use the LazyDetector template, I couldn't change the RID constructor without changing the LazyDetector template, and I didn't want to mess around with any of the other Detectors.
So, then I tried just adding a Triple member to the ROCMToolChain class for the Host Triple, and initializing it in its constructor. Which works. But it is essentially ends up being the same (or nearly the same) as just passing a flag, and doing the "work" in the ROCMToolChain constructor. If I use a member Triple for Host, I would still need to check it in the ROCMToolChain ctor, and tell its RID that the Host is Windows, before having the RID generate the search paths.
There was a problem hiding this comment.
The RID already has the host triple available I thought
There was a problem hiding this comment.
Right, we call it "HostTriple", but in the case for a Device side ToolChain, the triple passed to the TC's ctor (which is the same triple passed to the RID ctor) is going to be the device triple, which is always (I think) amdgcn-amd-amdhsa, or at least definitely not Windows. We can use the TC's triple for removing the linux search paths for Host side. But the problem is that this doesn't work when we create the Device TC, and it's RID.
There was a problem hiding this comment.
Do we actually require the ROCm installation from the device's TC? I guess it'd be used when making the device interface? The HIPAMDToolChain has a reference to the host toolchain, so I'd argue that we should use that one's ROCm installation and never use the device one.
There was a problem hiding this comment.
@jhuber6 So I spent some time trying to rework this to do just what you suggested (have the HIPAMDToolChain, just use the HostTC's RocmInstallation), because it is a good idea :-) But ... I ran into design issues. The member "HostTC" of HIPAMDToolChain, is a "const ToolChain &". And the "ToolChain" class does not have a "RocmInstallation" member, it is a member of the child class "Generic_GCC". We could cast "HostTC" reference to a Generic_GCC object reference (just in the HIPAMDToolClass), but that solution doesn't feel very pretty either.
There was a problem hiding this comment.
We do stuff like this in a ton of places,
clang/lib/Driver/ToolChains/Cuda.cpp
387: static_cast<const toolchains::NVPTXToolChain &>(getToolChain());
533: static_cast<const toolchains::CudaToolChain &>(getToolChain());
584: static_cast<const toolchains::NVPTXToolChain &>(getToolChain());
There was a problem hiding this comment.
ah ... ok. :-)
Change-Id: Ia0b44eb1069fa631a6d5156cf5881c978e23b62d
david-salinas
force-pushed
the
rocm_detect_windows
branch
from
a5a8499 to
c869225
Compare
jhuber6
left a comment
jhuber6
left a comment
There was a problem hiding this comment.
Is there an issue with simply using the HostTC for everything? I feel like that's the solution to this mess, since the HostTC would always know whether or not the target is Windows without us needing to forward a bunch of stuff.
Yes, that would work too. But currently the HostTC is only available/accessible in HIPAMDToolChain. With the way we instantiate the RID, we need to tell the RID ctor to hold off on creating the Search Paths if the HostTC is Windows. And the RID ctor is called all the way up in the Generic_GCC ctor. We would still have to pass the HostTC all the way through class hierarchy (HIPAMDToolChain -> ROCMToolChin -> AMDGPUToolChain -> Generic_ELF). My thinking was that passing a bool would be a little less intrusive/heavy of a change. Though, we would only be passing a reference to the HostTC. |
Maybe I'm missing something, both the Host / Device cases will initialize the ROCm detector, however I think for the GPU case this should be a trivial exit without doing any work. The uses of the ROCm detector should then be localized inside the ROCm tools I would think, at which case it's just a matter of reaching into the host toolchain. The AMDGPUToolChain does not have a host toolchain because it's the base class without ROCm information so it shouldn't have any dependencies on it. |
|
Okay, so it definitely uses the device side, however the ROCmToolChain does not take a host ToolChain. I'm wondering why the CUDA toolchain doesn't have this issue, it checks the HostTriple for Windows just fine. |
4 participants
No description provided.