r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

Thu Jul 19 06:43:41 PDT 2018

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 at 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