Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flang][Driver][Offload] Support -Xoffload-linker argument in Flang #109907

Merged
merged 8 commits into from
Sep 27, 2024

Conversation

agozillon
Copy link
Contributor

The -Xoffload-linker command allows forwarding of linker commands to the clang-linker-wrapper used for linking offload libraries into the resulting offload binaries amongst other tasks. This is a rather useful command to have to support the offloading programming models flang-new currently aims to support (OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a recent addition and is used in conjunction with the Fortran OpenMP test suite there, which fails at the moment due to flang-new not recognizing the command, this fixes the issue. The alternative to this would of course be to setup the test config to avoid using this flag with Fortran, but I believe adding support of the flag to flang-new has more merit as having the same compatability/communication capabilities as Clang to the clang-linker-wrapper is important as it's a critical component of the offload pipeline, and the command will likely see more use in the near future.

@llvmbot llvmbot added clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category labels Sep 25, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 25, 2024

@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: None (agozillon)

Changes

The -Xoffload-linker command allows forwarding of linker commands to the clang-linker-wrapper used for linking offload libraries into the resulting offload binaries amongst other tasks. This is a rather useful command to have to support the offloading programming models flang-new currently aims to support (OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a recent addition and is used in conjunction with the Fortran OpenMP test suite there, which fails at the moment due to flang-new not recognizing the command, this fixes the issue. The alternative to this would of course be to setup the test config to avoid using this flag with Fortran, but I believe adding support of the flag to flang-new has more merit as having the same compatability/communication capabilities as Clang to the clang-linker-wrapper is important as it's a critical component of the offload pipeline, and the command will likely see more use in the near future.


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

2 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+1)
  • (added) flang/test/Driver/xoffload-linker.f90 (+7)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 23bd686a85f526..da24bc3541abda 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1055,6 +1055,7 @@ def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
 def Xoffload_linker : JoinedAndSeparate<["-"], "Xoffload-linker">,
+  Visibility<[ClangOption, CLOption, FlangOption, DXCOption]>,
   HelpText<"Pass <arg> to the offload linkers or the ones identified by -<triple>">,
   MetaVarName<"<triple> <arg>">, Group<Link_Group>;
 def Xpreprocessor : Separate<["-"], "Xpreprocessor">, Group<Preprocessor_Group>,
diff --git a/flang/test/Driver/xoffload-linker.f90 b/flang/test/Driver/xoffload-linker.f90
new file mode 100644
index 00000000000000..886e649c1e8760
--- /dev/null
+++ b/flang/test/Driver/xoffload-linker.f90
@@ -0,0 +1,7 @@
+! Test the -Xoffload-linker flag that forwards link commands to the clang-linker-wrapper used
+! to help link offloading device libraries
+
+! RUN:   %flang -### --target=x86_64-unknown-linux-gnu -fopenmp --offload-arch=gfx90a \
+! RUN:      -Xoffload-linker a %s 2>&1 | FileCheck %s --check-prefix=CHECK-XLINKER
+
+! CHECK-XLINKER: -device-linker=a{{.*}}--

@tblah
Copy link
Contributor

tblah commented Sep 25, 2024

The CI failures look relevant. The problem isn't immediately apparent to me from the diff (which looks good to me).

@jhuber6
Copy link
Contributor

jhuber6 commented Sep 25, 2024

Maybe it's the double dashes after the check? I guess while we're at it might as well check the -Xoffload-linker-amdgcn-amd-amdhsa format as well.

@agozillon
Copy link
Contributor Author

hm, weird it passed on my system and I made sure it was running by intentionally failing it, I'll have a look into it and address it! And I'll add the other format as well as an additional test thank you both.

@agozillon
Copy link
Contributor Author

Added the new suggested test and removed the "--" couldn't see anything else that might lead to failure, but we'll see how the CI feels about it, if it doesn't like it please ignore the spam (and my appologies for it) on the PR as I try to work out what the CI wants from me for it to pass as it unfortunately passes locally which makes things hard to track!


end program
! CHECK-XLINKER-AMDGCN: -device-linker=a{{.*}}-device-linker=amdgcn-amd-amdhsa=b{{.*}}--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is there a -- here


! RUN: %flang -### --target=x86_64-unknown-linux-gnu -fopenmp --offload-arch=gfx90a -Xoffload-linker a -Xoffload-linker-amdgcn-amd-amdhsa b %s 2>&1 | FileCheck %s --check-prefixes=CHECK-XLINKER-AMDGCN
! CHECK-XLINKER: -device-linker=a{{.*}}-
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a - here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried it with multiple variations to see if the CI is happy with it, wouldn't focus too much on what's actually in the test at the moment until I work out the issue the CI has with the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. currently. copy pasted a similar passing test at the moment to see if the CI still takes issue with it.

The -Xoffload-linker command allows forwarding of linker commands to the
clang-linker-wrapper used for linking offload libraries into the resulting offload
binaries amongst other tasks. This is a rather useful command to have to support
the offloading programming models flang-new currently aims to support
(OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a recent addition and is
used in conjunction with the Fortran OpenMP test suite there, which fails at the
moment due to flang-new not recognizing the command, this fixes the issue. The
alternative to this would of course be to setup the test config to avoid using this
flag with Fortran, but I believe adding support of the flag to flang-new has more
merit as having the same compatability/communication capabilities as Clang to
the clang-linker-wrapper is important as it's a critical component of the offload
pipeline, and the command will likely see more use in the near future.
@agozillon
Copy link
Contributor Author

agozillon commented Sep 26, 2024

Passed this time, seems the addition of : "{{[^"]}}clang-linker-wrapper{{.}}" worked for both the CI and my local machine so that's good. Would love an additional approval from either of you @tblah @banach-space please if possible and the PR seems reasonable of course! P.S.. Thank you very much for the fix suggestion @jhuber6

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

@agozillon agozillon merged commit 3625f9f into llvm:main Sep 27, 2024
8 checks passed
s-perron pushed a commit to s-perron/llvm-project that referenced this pull request Sep 27, 2024
…lvm#109907)

The -Xoffload-linker command allows forwarding of linker commands to the
clang-linker-wrapper used for linking offload libraries into the
resulting offload binaries amongst other tasks. This is a rather useful
command to have to support the offloading programming models flang-new
currently aims to support (OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a
recent addition and is used in conjunction with the Fortran OpenMP test
suite there, which fails at the moment due to flang-new not recognizing
the command, this fixes the issue. The alternative to this would of
course be to setup the test config to avoid using this flag with
Fortran, but I believe adding support of the flag to flang-new has more
merit as having the same compatability/communication capabilities as
Clang to the clang-linker-wrapper is important as it's a critical
component of the offload pipeline, and the command will likely see more
use in the near future.
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Sep 27, 2024
…lvm#109907)

The -Xoffload-linker command allows forwarding of linker commands to the
clang-linker-wrapper used for linking offload libraries into the
resulting offload binaries amongst other tasks. This is a rather useful
command to have to support the offloading programming models flang-new
currently aims to support (OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a
recent addition and is used in conjunction with the Fortran OpenMP test
suite there, which fails at the moment due to flang-new not recognizing
the command, this fixes the issue. The alternative to this would of
course be to setup the test config to avoid using this flag with
Fortran, but I believe adding support of the flag to flang-new has more
merit as having the same compatability/communication capabilities as
Clang to the clang-linker-wrapper is important as it's a critical
component of the offload pipeline, and the command will likely see more
use in the near future.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…lvm#109907)

The -Xoffload-linker command allows forwarding of linker commands to the
clang-linker-wrapper used for linking offload libraries into the
resulting offload binaries amongst other tasks. This is a rather useful
command to have to support the offloading programming models flang-new
currently aims to support (OpenMP/OpenACC).

Currently this flag is utilised in the check-offload tests after a
recent addition and is used in conjunction with the Fortran OpenMP test
suite there, which fails at the moment due to flang-new not recognizing
the command, this fixes the issue. The alternative to this would of
course be to setup the test config to avoid using this flag with
Fortran, but I believe adding support of the flag to flang-new has more
merit as having the same compatability/communication capabilities as
Clang to the clang-linker-wrapper is important as it's a critical
component of the offload pipeline, and the command will likely see more
use in the near future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants