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

Thu Jul 19 06:55:20 PDT 2018

> Can you please elaborate on why you feel that this is problematic?

This change completely disables OpenMP parallelization on all systems those do not have libatomic library accessible.

So many people who earlier had working codes now cannot compiler any OpenMP program, even just print "Hello", that sound too huge restriction related to the fixed issue - save one compiler option for program with exotic atomic types.

-- Andrey

-----Original Message-----
From: cfe-commits [mailto:cfe-commits-bounces at lists.llvm.org] On Behalf Of Hal Finkel via cfe-commits
Sent: Thursday, July 19, 2018 4:44 PM
To: Jonas Hahnfeld <hahnjo at hahnjo.de>; Alexey Bataev <a.bataev at hotmail.com>
Cc: cfe-commits at lists.llvm.org
Subject: Re: r336467 - [OPENMP] Fix PR38026: Link -latomic when -fopenmp is used.

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/G
>> nu.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?r
>> ev=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

_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

--------------------------------------------------------------------
Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.