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.

Sign up for GitHub

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] allow -fopenmp= #86816

Merged
merged 3 commits into from
Apr 5, 2024
Merged

[FLANG] allow -fopenmp= #86816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
372995f
[FLANG] allow -fopenmp=
Leporacanthicus Mar 15, 2024
5cdd57d
Fix formatting of Flang.cpp
Leporacanthicus Mar 27, 2024
adec70d
Update with warning and tests to check for warning
Leporacanthicus Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3403,7 +3403,8 @@ defm openmp_extensions: BoolFOption<"openmp-extensions",
"Enable all Clang extensions for OpenMP directives and clauses">,
NegFlag<SetFalse, [NoArgumentUnused], [ClangOption, CC1Option],
"Disable all Clang extensions for OpenMP directives and clauses">>;
def fopenmp_EQ : Joined<["-"], "fopenmp=">, Group<f_Group>;
def fopenmp_EQ : Joined<["-"], "fopenmp=">, Group<f_Group>,
Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>;
def fopenmp_use_tls : Flag<["-"], "fopenmp-use-tls">, Group<f_Group>,
Flags<[NoArgumentUnused, HelpHidden]>;
def fnoopenmp_use_tls : Flag<["-"], "fnoopenmp-use-tls">, Group<f_Group>,
Expand Down
28 changes: 26 additions & 2 deletions clang/lib/Driver/ToolChains/Flang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ void Flang::addFortranDialectOptions(const ArgList &Args,
Args.addAllArgs(CmdArgs, {options::OPT_ffixed_form,
options::OPT_ffree_form,
options::OPT_ffixed_line_length_EQ,
options::OPT_fopenmp,
options::OPT_fopenmp_version_EQ,
options::OPT_fopenacc,
options::OPT_finput_charset_EQ,
options::OPT_fimplicit_none,
Expand Down Expand Up @@ -764,6 +762,32 @@ void Flang::ConstructJob(Compilation &C, const JobAction &JA,
// Add other compile options
addOtherOptions(Args, CmdArgs);

// Forward flags for OpenMP. We don't do this if the current action is an
// device offloading action other than OpenMP.
if (Args.hasFlag(options::OPT_fopenmp, options::OPT_fopenmp_EQ,
options::OPT_fno_openmp, false) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think -fno-openmp is visible in the flang driver. Maybe it should be.

(JA.isDeviceOffloading(Action::OFK_None) ||
JA.isDeviceOffloading(Action::OFK_OpenMP))) {
switch (D.getOpenMPRuntime(Args)) {
case Driver::OMPRT_OMP:
case Driver::OMPRT_IOMP5:
// Clang can generate useful OpenMP code for these two runtime libraries.
CmdArgs.push_back("-fopenmp");
Args.AddAllArgs(CmdArgs, options::OPT_fopenmp_version_EQ);

// FIXME: Clang supports a whole bunch more flags here.
tblah marked this conversation as resolved.
Show resolved Hide resolved
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we produce some sort of error in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, clang::Driver::getOpenMPRuntime does output an error (which we should test for), but currently we appear not to support libgomp, which clang does and so an error would not be produced in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what Clang does in this place. I'm not sure I understand why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it definitely is done this way on purpose - see clang/test/Driver/fopenmp.c (only "interesting" lines)

// RUN: %clang -target x86_64-linux-gnu -fopenmp=libgomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CC1-NO-OPENMP
...
// CHECK-CC1-OPENMP: "-cc1"
// CHECK-CC1-OPENMP: "-fopenmp"
//
// CHECK-CC1-NO-OPENMP: "-cc1"
// CHECK-CC1-NO-OPENMP-NOT: "-fopenmp"

We can certainly add a warning or error, but Clang doesn't... :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Clang might work differently internally. If we don't pass -fopenmp to the flang frontend driver, the parser will treat the openmp directives as comments. Which in many programs won't lead to any compilation errors but the program could produce different results (e.g. reductions won't happen)

Copy link
Contributor

Choose a reason for hiding this comment

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

Was leaving out OMPRT_GOMP deliberate? If so, please could you comment here or in the commit message explaining why and update the test not to be checking for libgomp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, behaving the same way as clang here.

Copy link
Contributor

Choose a reason for hiding this comment

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

If OMPRT_GOMP doesn't need -fopenmp for some reason then I don't think we understand how it works. Maybe we should prevent its use with flang for now?

// By default, if Clang doesn't know how to generate useful OpenMP code
// for a specific runtime library, we just don't pass the '-fopenmp' flag
// down to the actual compilation.
// FIXME: It would be better to have a mode which *only* omits IR
// generation based on the OpenMP support so that we get consistent
// semantic analysis, etc.
break;
}
}

// Offloading related options
addOffloadOptions(C, Inputs, JA, Args, CmdArgs);

Expand Down
60 changes: 60 additions & 0 deletions flang/test/Driver/fopenmp.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libgomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-NO-OPENMP
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libiomp5 -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-apple-darwin -fopenmp=libomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-apple-darwin -fopenmp=libgomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-NO-OPENMP
! RUN: %flang -target x86_64-apple-darwin -fopenmp=libiomp5 -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-freebsd -fopenmp=libomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-freebsd -fopenmp=libgomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-NO-OPENMP
! RUN: %flang -target x86_64-freebsd -fopenmp=libiomp5 -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libgomp -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-NO-OPENMP
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libiomp5 -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-FC1-OPENMP

! CHECK-FC1-OPENMP: "-fc1"
! CHECK-FC1-OPENMP: "-fopenmp"
!
! CHECK-FC1-NO-OPENMP: "-fc1"
! CHECK-FC1-NO-OPENMP-NOT: "-fopenmp"
!
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-OMP
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libgomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-GOMP --check-prefix=CHECK-LD-GOMP-RT
! RUN: %flang -target x86_64-linux-gnu -fopenmp=libiomp5 %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-IOMP5
!
! RUN: %flang -target x86_64-darwin -fopenmp=libomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-OMP
! RUN: %flang -target x86_64-darwin -fopenmp=libgomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-GOMP --check-prefix=CHECK-LD-GOMP-NO-RT
! RUN: %flang -target x86_64-darwin -fopenmp=libiomp5 %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-IOMP5
!
! RUN: %flang -target x86_64-freebsd -fopenmp=libomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-OMP
! RUN: %flang -target x86_64-freebsd -fopenmp=libgomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-GOMP --check-prefix=CHECK-LD-GOMP-NO-RT
! RUN: %flang -target x86_64-freebsd -fopenmp=libiomp5 %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-IOMP5
!
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-OMP
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libgomp %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-GOMP --check-prefix=CHECK-LD-GOMP-NO-RT
! RUN: %flang -target x86_64-windows-gnu -fopenmp=libiomp5 %s -o %t -### 2>&1 | FileCheck %s --check-prefix=CHECK-LD-IOMP5MD
!
! CHECK-LD-OMP: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-OMP: "-lomp"
!
! CHECK-LD-GOMP: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-GOMP: "-lgomp"
! CHECK-LD-GOMP-RT: "-lrt"
! CHECK-LD-GOMP-NO-RT-NOT: "-lrt"
!
! CHECK-LD-IOMP5: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-IOMP5: "-liomp5"
!
! CHECK-LD-IOMP5MD: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-IOMP5MD: "-liomp5md"
!
! We'd like to check that the default is sane, but until we have the ability
! to *always* semantically analyze OpenMP without always generating runtime
! calls (in the event of an unsupported runtime), we don't have a good way to
! test the CC1 invocation. Instead, just ensure we do eventually link *some*
! OpenMP runtime.
!
! CHECK-LD-ANY: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-ANY: "-l{{(omp|gomp|iomp5)}}"
!
! CHECK-LD-ANYMD: "{{.*}}ld{{(.exe)?}}"
! CHECK-LD-ANYMD: "-l{{(omp|gomp|iomp5md)}}"
9 changes: 7 additions & 2 deletions flang/test/Driver/omp-driver-offload.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,14 @@
! RUN: --target=aarch64-unknown-linux-gnu \
! RUN: | FileCheck %s --check-prefix=OPENMP-OFFLOAD-ARGS
! OPENMP-OFFLOAD-ARGS: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}}.f90"
! OPENMP-OFFLOAD-ARGS-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa" {{.*}} "-fopenmp" {{.*}} "-fopenmp-host-ir-file-path" "{{.*}}.bc" "-fopenmp-is-target-device" {{.*}}.f90"
! OPENMP-OFFLOAD-ARGS-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa"
! OPENMP-OFFLOAD-ARGS-SAME: "-fopenmp"
! OPENMP-OFFLOAD-ARGS-SAME: "-fopenmp-host-ir-file-path" "{{.*}}.bc" "-fopenmp-is-target-device"
! OPENMP-OFFLOAD-ARGS-SAME: {{.*}}.f90"
! OPENMP-OFFLOAD-ARGS: "{{[^"]*}}clang-offload-packager{{.*}}" {{.*}} "--image=file={{.*}}.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"
! OPENMP-OFFLOAD-ARGS-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}} "-fembed-offload-object={{.*}}.out" {{.*}}.bc"
! OPENMP-OFFLOAD-ARGS-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" "aarch64-unknown-linux-gnu"
! OPENMP-OFFLOAD-ARGS-SAME: "-fopenmp"
! OPENMP-OFFLOAD-ARGS-SAME: "-fembed-offload-object={{.*}}.out" {{.*}}.bc"

! Test -fopenmp with offload for RTL Flag Options
! RUN: %flang -### %s -o %t 2>&1 \
Expand Down
Loading