On 07/16/2018 01:19 PM, Jonas Hahnfeld wrote: > [ Moving discussion from https://reviews.llvm.org/D49386 to the > relevant comment on cfe-commits, CC'ing Hal who commented on the > original issue ] > > Is this change really a good idea? It always requires libatomic for > all OpenMP applications, even if there is no 'omp atomic' directive or > all of them can be lowered to atomic instructions that don't require a > runtime library. I'd argue that it's a larger restriction than the > problem it solves.
Can you please elaborate on why you feel that this is problematic? > Per https://clang.llvm.org/docs/Toolchain.html#libatomic-gnu the user > is expected to manually link -latomic whenever Clang can't lower > atomic instructions - including C11 atomics and C++ atomics. In my > opinion OpenMP is just another abstraction that doesn't require a > special treatment. From my perspective, because we instruct our users that all you need to do in order to enable OpenMP is pass -fopenmp flags during compiling and linking. The user should not need to know or care about how atomics are implemented. It's not clear to me that our behavior for C++ atomics is good either. From the documentation, it looks like the rationale is to avoid choosing between the GNU libatomic implementation and the compiler-rt implementation? We should probably make a default choice and provide a flag to override. That would seem more user-friendly to me. -Hal > > Thoughts? > Jonas > > On 2018-07-06 23:13, Alexey Bataev via cfe-commits wrote: >> Author: abataev >> Date: Fri Jul 6 14:13:41 2018 >> New Revision: 336467 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=336467&view=rev >> Log: >> [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used. >> >> On Linux atomic constructs in OpenMP require libatomic library. Patch >> links libatomic when -fopenmp is used. >> >> Modified: >> cfe/trunk/lib/Driver/ToolChains/Gnu.cpp >> cfe/trunk/test/OpenMP/linking.c >> >> Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=336467&r1=336466&r2=336467&view=diff >> >> ============================================================================== >> >> --- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original) >> +++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Fri Jul 6 14:13:41 2018 >> @@ -479,6 +479,7 @@ void tools::gnutools::Linker::ConstructJ >> >> bool WantPthread = Args.hasArg(options::OPT_pthread) || >> Args.hasArg(options::OPT_pthreads); >> + bool WantAtomic = false; >> >> // FIXME: Only pass GompNeedsRT = true for platforms with >> libgomp that >> // require librt. Most modern Linux platforms do, but some may >> not. >> @@ -487,13 +488,16 @@ void tools::gnutools::Linker::ConstructJ >> /* GompNeedsRT= */ true)) >> // OpenMP runtimes implies pthreads when using the GNU >> toolchain. >> // FIXME: Does this really make sense for all GNU toolchains? >> - WantPthread = true; >> + WantAtomic = WantPthread = true; >> >> AddRunTimeLibs(ToolChain, D, CmdArgs, Args); >> >> if (WantPthread && !isAndroid) >> CmdArgs.push_back("-lpthread"); >> >> + if (WantAtomic) >> + CmdArgs.push_back("-latomic"); >> + >> if (Args.hasArg(options::OPT_fsplit_stack)) >> CmdArgs.push_back("--wrap=pthread_create"); >> >> >> Modified: cfe/trunk/test/OpenMP/linking.c >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/linking.c?rev=336467&r1=336466&r2=336467&view=diff >> >> ============================================================================== >> >> --- cfe/trunk/test/OpenMP/linking.c (original) >> +++ cfe/trunk/test/OpenMP/linking.c Fri Jul 6 14:13:41 2018 >> @@ -8,14 +8,14 @@ >> // RUN: | FileCheck --check-prefix=CHECK-LD-32 %s >> // CHECK-LD-32: "{{.*}}ld{{(.exe)?}}" >> // CHECK-LD-32: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]" >> -// CHECK-LD-32: "-lpthread" "-lc" >> +// CHECK-LD-32: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp -target x86_64-unknown-linux -rtlib=platform \ >> // RUN: | FileCheck --check-prefix=CHECK-LD-64 %s >> // CHECK-LD-64: "{{.*}}ld{{(.exe)?}}" >> // CHECK-LD-64: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]" >> -// CHECK-LD-64: "-lpthread" "-lc" >> +// CHECK-LD-64: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp=libgomp -target i386-unknown-linux >> -rtlib=platform \ >> @@ -27,7 +27,7 @@ >> // SIMD-ONLY2-NOT: liomp >> // CHECK-GOMP-LD-32: "{{.*}}ld{{(.exe)?}}" >> // CHECK-GOMP-LD-32: "-lgomp" "-lrt" >> -// CHECK-GOMP-LD-32: "-lpthread" "-lc" >> +// CHECK-GOMP-LD-32: "-lpthread" "-latomic" "-lc" >> >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 >> -fopenmp-simd -target i386-unknown-linux -rtlib=platform | FileCheck >> --check-prefix SIMD-ONLY2 %s >> // SIMD-ONLY2-NOT: lgomp >> @@ -39,21 +39,21 @@ >> // RUN: | FileCheck --check-prefix=CHECK-GOMP-LD-64 %s >> // CHECK-GOMP-LD-64: "{{.*}}ld{{(.exe)?}}" >> // CHECK-GOMP-LD-64: "-lgomp" "-lrt" >> -// CHECK-GOMP-LD-64: "-lpthread" "-lc" >> +// CHECK-GOMP-LD-64: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp -target i386-unknown-linux -rtlib=platform \ >> // RUN: | FileCheck --check-prefix=CHECK-IOMP5-LD-32 %s >> // CHECK-IOMP5-LD-32: "{{.*}}ld{{(.exe)?}}" >> // CHECK-IOMP5-LD-32: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]" >> -// CHECK-IOMP5-LD-32: "-lpthread" "-lc" >> +// CHECK-IOMP5-LD-32: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp -target x86_64-unknown-linux -rtlib=platform \ >> // RUN: | FileCheck --check-prefix=CHECK-IOMP5-LD-64 %s >> // CHECK-IOMP5-LD-64: "{{.*}}ld{{(.exe)?}}" >> // CHECK-IOMP5-LD-64: "-l[[DEFAULT_OPENMP_LIB:[^"]*]]" >> -// CHECK-IOMP5-LD-64: "-lpthread" "-lc" >> +// CHECK-IOMP5-LD-64: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp=lib -target i386-unknown-linux \ >> @@ -71,7 +71,7 @@ >> // RUN: | FileCheck --check-prefix=CHECK-LD-OVERRIDE-32 %s >> // CHECK-LD-OVERRIDE-32: "{{.*}}ld{{(.exe)?}}" >> // CHECK-LD-OVERRIDE-32: "-lgomp" "-lrt" >> -// CHECK-LD-OVERRIDE-32: "-lpthread" "-lc" >> +// CHECK-LD-OVERRIDE-32: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp -fopenmp=libgomp -target x86_64-unknown-linux \ >> @@ -79,7 +79,7 @@ >> // RUN: | FileCheck --check-prefix=CHECK-LD-OVERRIDE-64 %s >> // CHECK-LD-OVERRIDE-64: "{{.*}}ld{{(.exe)?}}" >> // CHECK-LD-OVERRIDE-64: "-lgomp" "-lrt" >> -// CHECK-LD-OVERRIDE-64: "-lpthread" "-lc" >> +// CHECK-LD-OVERRIDE-64: "-lpthread" "-latomic" "-lc" >> // >> // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \ >> // RUN: -fopenmp=libomp -target x86_64-msvc-win32 -rtlib=platform \ >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits -- Hal Finkel Lead, Compiler Technology and Programming Languages Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
