[Flang][AMDGPU] Add rocm-path flag #88190

DominikAdamski · 2024-04-09T20:35:15Z

ROCm installation path is used for finding and automatically linking required bitcode libraries for OpenMP AMDGPU offload.

llvmbot · 2024-04-09T20:35:44Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: Dominik Adamski (DominikAdamski)

Changes

ROCm installation path is used for finding and automatically linking required bitcode libraries for OpenMP AMDGPU offload.

Full diff: https://github.com/llvm/llvm-project/pull/88190.diff

5 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+2-1)
(modified) clang/lib/Driver/ToolChains/Flang.cpp (+4)
(modified) flang/test/Driver/driver-help-hidden.f90 (+1)
(modified) flang/test/Driver/driver-help.f90 (+1)
(modified) flang/test/Driver/omp-driver-offload.f90 (+21)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 827d9d7c0c18e4..64ffb15939bb15 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1340,7 +1340,8 @@ def hip_link : Flag<["--"], "hip-link">, Group<opencl_Group>,
   HelpText<"Link clang-offload-bundler bundles for HIP">;
 def no_hip_rt: Flag<["-"], "no-hip-rt">, Group<hip_Group>,
   HelpText<"Do not link against HIP runtime libraries">;
-def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group<hip_Group>,
+def rocm_path_EQ : Joined<["--"], "rocm-path=">,
+  Visibility<[FlangOption]>, Group<hip_Group>,
   HelpText<"ROCm installation path, used for finding and automatically linking required bitcode libraries.">;
 def hip_path_EQ : Joined<["--"], "hip-path=">, Group<hip_Group>,
   HelpText<"HIP runtime installation path, used for finding HIP version and adding HIP include path.">;
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 2c83f70eb7887e..75e4ead81e43ed 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -342,6 +342,10 @@ void Flang::AddAMDGPUTargetArgs(const ArgList &Args,
     StringRef Val = A->getValue();
     CmdArgs.push_back(Args.MakeArgString("-mcode-object-version=" + Val));
   }
+
+  // Check ROCm path if specified
+  const ToolChain &TC = getToolChain();
+  TC.getDeviceLibs(Args);
 }
 
 void Flang::addTargetOptions(const ArgList &Args,
diff --git a/flang/test/Driver/driver-help-hidden.f90 b/flang/test/Driver/driver-help-hidden.f90
index 48f48f5384fdc5..10b15fb454b9aa 100644
--- a/flang/test/Driver/driver-help-hidden.f90
+++ b/flang/test/Driver/driver-help-hidden.f90
@@ -144,6 +144,7 @@
 ! CHECK-NEXT: -print-target-triple    Print the normalized target triple
 ! CHECK-NEXT: -pthread                Support POSIX threads in generated code
 ! CHECK-NEXT: -P                      Disable linemarker output in -E mode
+! CHECK-NEXT: --rocm-path=<value> ROCm installation path, used for finding and automatically linking required bitcode libraries.
 ! CHECK-NEXT: -Rpass-analysis=<value> Report transformation analysis from optimization passes whose name matches the given POSIX regular expression
 ! CHECK-NEXT: -Rpass-missed=<value>   Report missed transformations by optimization passes whose name matches the given POSIX regular expression
 ! CHECK-NEXT: -Rpass=<value>          Report transformations performed by optimization passes whose name matches the given POSIX regular expression
diff --git a/flang/test/Driver/driver-help.f90 b/flang/test/Driver/driver-help.f90
index 38f74395a678ab..ed5af2a68eb044 100644
--- a/flang/test/Driver/driver-help.f90
+++ b/flang/test/Driver/driver-help.f90
@@ -132,6 +132,7 @@
 ! HELP-NEXT: -print-target-triple    Print the normalized target triple
 ! HELP-NEXT: -pthread                Support POSIX threads in generated code
 ! HELP-NEXT: -P                      Disable linemarker output in -E mode
+! HELP-NEXT:  --rocm-path=<value> ROCm installation path, used for finding and automatically linking required bitcode libraries.
 ! HELP-NEXT: -Rpass-analysis=<value> Report transformation analysis from optimization passes whose name matches the given POSIX regular expression
 ! HELP-NEXT: -Rpass-missed=<value>   Report missed transformations by optimization passes whose name matches the given POSIX regular expression
 ! HELP-NEXT: -Rpass=<value>          Report transformations performed by optimization passes whose name matches the given POSIX regular expression
diff --git a/flang/test/Driver/omp-driver-offload.f90 b/flang/test/Driver/omp-driver-offload.f90
index 7e9a73627cd757..836dcfc85eb9de 100644
--- a/flang/test/Driver/omp-driver-offload.f90
+++ b/flang/test/Driver/omp-driver-offload.f90
@@ -201,3 +201,24 @@
 ! RUN:      -nogpulibc %s 2>&1 \
 ! RUN:   | FileCheck --check-prefix=NO-LIBC-GPU-AMDGPU %s
 ! NO-LIBC-GPU-AMDGPU-NOT: "-lcgpu-amdgpu"
+
+! RUN:   rm -rf %t/Inputs
+
+! RUN:   not %flang -### -v --target=x86_64-unknown-linux-gnu -fopenmp  \
+! RUN:      --offload-arch=gfx900 \
+! RUN:      --rocm-path=%t/Inputs/rocm %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=ROCM-PATH-NOT-FOUND %s
+! ROCM-PATH-NOT-FOUND: error: cannot find ROCm device library;
+
+! RUN:   rm -rf %t/Inputs
+! RUN:   mkdir -p %t/Inputs
+! RUN:   cp -r %S/../../../clang/test/Driver/Inputs/rocm %t/Inputs
+! RUN:   mkdir -p %t/Inputs/rocm/share/hip
+! RUN:   mkdir -p %t/Inputs/rocm/hip
+! RUN: mv %t/Inputs/rocm/bin/.hipVersion %t/Inputs/rocm/share/hip/version
+
+! RUN:   %flang -### -v --target=x86_64-unknown-linux-gnu -fopenmp  \
+! RUN:      --offload-arch=gfx900 \
+! RUN:      --rocm-path=%t/Inputs/rocm %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=ROCM-PATH %s
+! ROCM-PATH: Found HIP installation: {{.*Inputs.*rocm}}, version 3.6.20214-a2917cd

jhuber6 · 2024-04-09T20:41:39Z

flang/test/Driver/omp-driver-offload.f90

+
+! RUN:   not %flang -### -v --target=x86_64-unknown-linux-gnu -fopenmp  \
+! RUN:      --offload-arch=gfx900 \
+! RUN:      --rocm-path=%t/Inputs/rocm %s 2>&1 \


Where does this live? There's one in clang but not flang.

I'm assuming you mean %S as well.

This test checks if flang reports an error if ROCm path doesn't exist. The next test checks positive scenario.

Even in that case I don't think %t is right, should probably just point it to some non-existant path off the current directory %S.

jhuber6 · 2024-04-09T20:42:57Z

clang/lib/Driver/ToolChains/Flang.cpp

+  // Check ROCm path if specified
+  const ToolChain &TC = getToolChain();
+  TC.getDeviceLibs(Args);


What does this have to do with adding --rocm-path? Does flang correctly inherit the RocmInstallationDetector logic once specified?

I need it to invoke AMDGPUOpenMPToolChain::getDeviceLibs to check if specified ROCm path is correct.

Doing clang -v will show you something like

Found HIP installation: /opt/rocm, version 6.0.32831

Can we not check that way?

Please look at the recent change:
5b10623
Is this solution better?

jhuber6 · 2024-04-09T20:43:41Z

clang/include/clang/Driver/Options.td

@@ -1340,7 +1340,8 @@ def hip_link : Flag<["--"], "hip-link">, Group<opencl_Group>,
  HelpText<"Link clang-offload-bundler bundles for HIP">;
 def no_hip_rt: Flag<["-"], "no-hip-rt">, Group<hip_Group>,
  HelpText<"Do not link against HIP runtime libraries">;
-def rocm_path_EQ : Joined<["--"], "rocm-path=">, Group<hip_Group>,
+def rocm_path_EQ : Joined<["--"], "rocm-path=">,
+  Visibility<[FlangOption]>, Group<hip_Group>,


Just making sure, but adding the explicit visibility here doesn't override the standard clang ones?

ninja check-clang does not report any error. Is it enough?

Yeah, should be fine.

ROCm installation path is used for finding and automatically linking required bitcode libraries.

DominikAdamski · 2024-04-10T12:26:03Z

clang/lib/Driver/ToolChains/Flang.cpp

+  llvm::raw_string_ostream HIPInfo(HIPVersion);
+  TC.printVerboseInfo(HIPInfo);
+  llvm::StringRef HIPInfoStrRef(HIPInfo.str());
+  if (!HIPInfoStrRef.contains("Found HIP installation") &&


Note: I can extract the Found HIP installation string into a separate constant so that it can be used by both clang and flang.

What's the utility of this bit? Don't we already do this checking inside of the ROCm toolchain? Does Flang not use that?

Clang reports an error if --rocm-path points to an invalid directory. I need to do a similar check for Flang.

The ROCm toolchain checks rocm-path within function RocmInstallationDetector::checkCommonBitcodeLibs. This function is called inside ROCMToolChain::addClangTargetOptions or ROCMToolChain::getCommonDeviceLibNames. ROCMToolChain::getCommonDeviceLibNames is called by AMDGPUOpenMPToolChain::getDeviceLibs. I decided not to call ROCMToolChain::addClangTargetOptions because Flang does not support all Clang options. That's why I initially decided to call AMDGPUOpenMPToolChain::getDeviceLibs to check the ROCm path. The second (current) approach is a workaround to emit an error if rocm-path is specified incorrectly.

Do you know where clang emits that diagnostic? I'd assume it would be done during ROCm detection.

test command:
clang -fopenmp --offload-arch=gfx90a test.c

Function call:

clang/lib/Driver/ToolChains/Clang.cpp : void Clang::ConstructJob TC.addClangTargetOptions(Args, CmdArgs, JA.getOffloadingDeviceKind()); | V clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp: void AMDGPUOpenMPToolChain::addClangTargetOptions for (auto BCFile : getDeviceLibs(DriverArgs)) { | V clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp: AMDGPUOpenMPToolChain::getDeviceLibs ( | V if (!RocmInstallation->hasDeviceLibrary()) { getDriver().Diag(diag::err_drv_no_rocm_device_lib) << 0; return {}; }

Do we use these libraries at all yet?

No, we do not use them in Flang

jhuber6 · 2024-04-12T14:09:36Z

flang/test/Driver/omp-driver-offload.f90

+
+! RUN:   rm -rf %t/Inputs
+! RUN:   mkdir -p %t/Inputs
+! RUN:   cp -r %S/../../../clang/test/Driver/Inputs/rocm %t/Inputs


Can we just copy our own version here? Not a huge fan of pulling it out of Clang.

DominikAdamski requested review from banach-space and jhuber6 April 9, 2024 20:35

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Apr 9, 2024

jhuber6 reviewed Apr 9, 2024

View reviewed changes

DominikAdamski added 2 commits April 10, 2024 03:48

[Flang][AMDGPU] Add rocm-path flag

44def17

ROCm installation path is used for finding and automatically linking required bitcode libraries.

Remove extra new line

1d0ecee

DominikAdamski force-pushed the flang_rocm_path branch from 1c06c5a to 1d0ecee Compare April 10, 2024 09:18

DominikAdamski mentioned this pull request Apr 10, 2024

flang-new: error: unknown argument: --gcc-toolchain, --rocm-path, -frtlib-add-rpath #82553

Closed

DominikAdamski commented Apr 10, 2024

View reviewed changes

Applied remarks

b701de9

DominikAdamski force-pushed the flang_rocm_path branch from 5b10623 to b701de9 Compare April 11, 2024 11:50

DominikAdamski added 2 commits April 12, 2024 08:17

Applied remarks

22b0a3f

Merge branch 'main' into flang_rocm_path

005c175

jhuber6 reviewed Apr 12, 2024

View reviewed changes

copy rocm input for test

a7cf6c8

jhuber6 approved these changes Apr 12, 2024

View reviewed changes

DominikAdamski merged commit a27ab3f into llvm:main Apr 12, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flang][AMDGPU] Add rocm-path flag #88190

[Flang][AMDGPU] Add rocm-path flag #88190

DominikAdamski commented Apr 9, 2024

llvmbot commented Apr 9, 2024 •

edited

jhuber6 Apr 9, 2024

jhuber6 Apr 9, 2024

DominikAdamski Apr 9, 2024

jhuber6 Apr 10, 2024

jhuber6 Apr 9, 2024

DominikAdamski Apr 9, 2024

jhuber6 Apr 9, 2024

DominikAdamski Apr 10, 2024

jhuber6 Apr 9, 2024

DominikAdamski Apr 9, 2024

jhuber6 Apr 9, 2024

DominikAdamski Apr 10, 2024

jhuber6 Apr 11, 2024

DominikAdamski Apr 11, 2024

jhuber6 Apr 11, 2024

DominikAdamski Apr 12, 2024

jhuber6 Apr 12, 2024

DominikAdamski Apr 12, 2024

jhuber6 Apr 12, 2024

DominikAdamski Apr 12, 2024

[Flang][AMDGPU] Add rocm-path flag #88190

[Flang][AMDGPU] Add rocm-path flag #88190

Conversation

DominikAdamski commented Apr 9, 2024

llvmbot commented Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Apr 9, 2024 •

edited