[Flang][Driver] Introduce -fopenmp-targets offloading option #100152
Conversation
skatrak
requested review from
ergawy,
Meinersbur,
jsjodin,
tblah,
kiranchandramohan,
abidh,
mjklemm,
raghavendhra,
agozillon,
DominikAdamski,
TIFitis,
kparzysz,
anchuraj and
bhandarkar-pranav
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-clang Author: Sergio Afonso (skatrak) ChangesThis patch modifies the flang driver to introduce the This option holds the list of offloading triples associated to the compilation and is used by clang to determine whether offloading calls should be generated for the host. Full diff: https://github.com/llvm/llvm-project/pull/100152.diff 3 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 69269cf7537b0..4ef7c81fbd9e4 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3530,7 +3530,7 @@ def fopenmp_use_tls : Flag<["-"], "fopenmp-use-tls">, Group<f_Group>,
def fnoopenmp_use_tls : Flag<["-"], "fnoopenmp-use-tls">, Group<f_Group>,
Flags<[NoArgumentUnused, HelpHidden]>, Visibility<[ClangOption, CC1Option]>;
def fopenmp_targets_EQ : CommaJoined<["-"], "fopenmp-targets=">,
- Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option, FlangOption]>,
+ Flags<[NoXarchOption]>, Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
HelpText<"Specify comma-separated list of triples OpenMP offloading targets to be supported">;
def fopenmp_relocatable_target : Flag<["-"], "fopenmp-relocatable-target">,
Group<f_Group>, Flags<[NoArgumentUnused, HelpHidden]>,
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index c4f2375c64034..b0a3528dce05d 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -12,6 +12,7 @@
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Driver/Options.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Frontend/Debug/Options.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
@@ -492,6 +493,18 @@ void Flang::addOffloadOptions(Compilation &C, const InputInfoList &Inputs,
if (Args.hasArg(options::OPT_nogpulib))
CmdArgs.push_back("-nogpulib");
}
+
+ // For all the host OpenMP offloading compile jobs we need to pass the targets
+ // information using -fopenmp-targets= option.
+ if (JA.isHostOffloading(Action::OFK_OpenMP)) {
+ SmallString<128> Targets("-fopenmp-targets=");
+
+ SmallVector<std::string, 4> Triples;
+ auto TCRange = C.getOffloadToolChains<Action::OFK_OpenMP>();
+ std::transform(TCRange.first, TCRange.second, std::back_inserter(Triples),
+ [](auto TC) { return TC.second->getTripleString(); });
+ CmdArgs.push_back(Args.MakeArgString(Targets + llvm::join(Triples, ",")));
+ }
}
static void addFloatingPointOptions(const Driver &D, const ArgList &Args,
diff --git a/flang/test/Driver/omp-driver-offload.f90 b/flang/test/Driver/omp-driver-offload.f90
index 6fb4f4eeeeca1..70fb8c7298e77 100644
--- a/flang/test/Driver/omp-driver-offload.f90
+++ b/flang/test/Driver/omp-driver-offload.f90
@@ -227,3 +227,20 @@
! FORCE-USM-OFFLOAD-SAME: "-fopenmp" "-fopenmp-force-usm"
! FORCE-USM-OFFLOAD-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa"
! FORCE-USM-OFFLOAD-SAME: "-fopenmp" "-fopenmp-force-usm"
+
+! RUN: %flang -S -### %s -o %t 2>&1 \
+! RUN: -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa \
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN: | FileCheck %s --check-prefix=OFFLOAD-TARGETS
+
+! RUN: %flang -S -### %s -o %t 2>&1 \
+! RUN: -fopenmp --offload-arch=gfx90a \
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN: | FileCheck %s --check-prefix=OFFLOAD-TARGETS
+
+! OFFLOAD-TARGETS: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu"
+! OFFLOAD-TARGETS-SAME: "-fopenmp-targets=amdgcn-amd-amdhsa"
+! OFFLOAD-TARGETS-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa"
+! OFFLOAD-TARGETS-NOT: -fopenmp-targets
+! OFFLOAD-TARGETS: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu"
+! OFFLOAD-TARGETS-SAME: "-fopenmp-targets=amdgcn-amd-amdhsa"
|
Thank you for the quick review. Yes, it's basically copied from clang, just like most of the other code we have in there, hopefully we can at some point refactor things a bit more. This is part of a PR stack, and the last one shows a use case for this: |
skatrak
force-pushed
the
users/skatrak/target-host-01-driver
branch
from
848937e to
a5d5c98
Compare
|
I'm struggling to fix the buildbot issue triggered by the new test in omp-driver-offload-amdgpu.f90. It was originally in omp-driver-offload.f90, I only moved it to a new file hoping that adding a "REQUIRES: amdgpu-registered-target" would address it. It seems like it doesn't. It fails with an exit code of 1, but with no output from FileCheck. I'm not able to reproduce this issue locally, as it passes as expected, so I'm not sure what the problem actually is. Does anyone have any pointers as to how to reproduce/resolve the issue? |
Driver tests should rarely ever need registered target restrictions. That check just ensures that the backend is enabled, which isn't required to create LLVM-IR, only GCN. |
|
I reproduced this locally. The output is: amdgpu-arch fails with: i.e., I don't have ROCm installed. |
|
|
Can you please provide more details about the design of the interface for setting the offload targets? Is the idea here to mirror clang's interface? While clang has a uniform design for the main target triple it doesn't really have such uniformity for the offload. Since flang is being newly developed it maybe useful to think whether the interface can be made more uniform. Are there any other language modes or targets that require such setting for offload? How would it look like from the user's perspective as a big picture to select the offload target for the programming models supported by flang? |
|
I think we can/should go with the "--offload" set of flags, and potentially have an "--offload-targets=..." version that clang is missing. The latter is mostly not needed, as --offload-arch=..." is pretty good, but there are cases you want to make it explicit to pass on additional arguments to tool chains. Calling it "openmp" is counter productive, IMHO, but the interface itself seems fine to me. |
|
Given the growing number of OpenMP and/or "offloading" flags, I agree with @AnastasiaStulova that it would be good to clarify the overall goal/design. That's not clear to me. Is there are reference implementation that Flang is meant to follow? For example Clang or GFortran? Could we re-use more of Clang logic here? There's also a question how various flags impact one another and that seems also confusing (see e.g. this question by @DominikAdamski ). Now, this is not really my area of expertise and I tend to stay hands-off. I rely on @jhuber6's expertise in the area :) But it does sound like it would be good to discuss the long term goals here. Btw, that's not a blocker for me for this particular change. |
|
Adding One related question I have - does Flang compile for one offload model or can it mix those (let's say compile sources with mixed OpenACC and OpenMP pragmas)? If mixing is allowed then it may also make sense to add a way of setting the target per programming model. But generic flag Btw does the offload target triple have identical format to the main triple? |
|
We could probably also add something like |
|
I'm not very knowledgeable on the set of offloading-related options clang supports, how those impact the set of invocations and options produced by the driver, or how these options eventually impact passes and codegen. Much less about potential shortcomings and improvements to the current approach, which appears to be what the discussion here is moving to. However, I can give a brief overview of what this PR stack is about. The flang-new driver is very much based on clang, only limited in the set of options it can currently handle and adding Fortran-specific ones. In terms of offloading, it also produces a separate compiler invocation for the host and the target device, based on the same machinery and driver options as far as I can tell. One current limitation we have noticed is that the host compilation for Flang will generate offloading calls ( This PR makes sure that it is also the case for flang-new that the In my opinion, the decision to potentially rename or move to a different set of offloading flags should be independent from this work, as it doesn't really add anything new, it only replicates one of the features clang already has. If a change to that is agreed upon, we can then update both clang and flang separately. If people agree, I can merge these changes and perhaps the more general discussion could be continued in discourse. |
I'm fine with supporting OpenACC and OpenMP offload at the same time, they use the same runtime and the semantics they impose might not conflict too much. That said, one target per "model" is not clear to me, both in terms of meaning and use case. |
I would agree that this discussion belongs to an RFC in discourse. I don't know if RFC should block the progress of this patch or not. The only issue I see is that this PR adds a new driver flag that may be used to compile the applications. Renaming the user visible flags is not that easy. But if they get renamed before any compiler release then it should be fine. |
Thank you for the comment. Actually, this PR doesn't add any new driver options to flang. It just enables the |
Ah yes sorry thanks for pointing out I overlooked that. Then I agree this seems not to affect the user interface... so there should be no concerns. |
skatrak
force-pushed
the
users/skatrak/target-host-01-driver
branch
from
04cac5b to
3861b28
Compare
|
Thank you all for your reviews. I think it should be ok to merge at this point, but I'll wait until tomorrow to give some time in case there are any remaining concerns about this change. |
This patch modifies the flang driver to introduce the `-fopenmp-targets` option to the frontend compiler invocations corresponding to the OpenMP host device on offloading-enabled compilations. This option holds the list of offloading triples associated to the compilation and is used by clang to determine whether offloading calls should be generated for the host.
skatrak
force-pushed
the
users/skatrak/target-host-01-driver
branch
from
3861b28 to
e59c38d
Compare
This patch modifies the flang driver to introduce the
-fopenmp-targetsoption to the frontend compiler invocations corresponding to the OpenMP host device on offloading-enabled compilations.This option holds the list of offloading triples associated to the compilation and is used by clang to determine whether offloading calls should be generated for the host.