[PATCH] D16614: Use "long long int" when checking whether atomics are supported.

[Vasileios Kalintiris via llvm-commits]

My understanding from the discussion so far is that it would be better to check whether we need 64bit atomics  only in libcxx.
I'm going to abandon this review request and update the libcxx one with the necessary logic for detecting support/lack of 64bit atomics.

Thanks.
________________________________________
From: llvm-commits [llvm-commits-bounces at lists.llvm.org] on behalf of Joerg Sonnenberger via llvm-commits [llvm-commits at lists.llvm.org]
Sent: 28 January 2016 13:18
To: llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D16614: Use "long long int" when checking whether atomics are supported.

On Thu, Jan 28, 2016 at 11:08:45AM +0000, Daniel Sanders wrote:
> > -----Original Message-----
> > From: Joerg Sonnenberger [mailto:joerg at britannica.bec.de]
> > Sent: 28 January 2016 00:35
> > To: Daniel Sanders
> > Cc: Vasileios Kalintiris; llvm-commits at lists.llvm.org
> > Subject: Re: [PATCH] D16614: Use "long long int" when checking whether
> > atomics are supported.
> >
> > On Wed, Jan 27, 2016 at 03:23:52PM +0000, Daniel Sanders wrote:
> > > > Yes, but emulations larging the native register width tend to be
> > > > significantly slower and raise questions about signal safety and the
> > > > like. That makes them quite important to avoid for a lot of "production"
> > > > code.
> > >
> > > It's true that users may want to avoid them for performance reasons but
> > > this doesn't mean we should fail to compile code that contains emulated
> > atomics.
> >
> > As I said, I believe that LLVM *should* fail to build when 64bit atomics
> > are used on 32bit platforms.
>
> I disagree with this argument on the grounds that code containing
> std::atomic<long long> is valid C++ and support for it is required by
> the C++ standard.

The standard allows a lot of stupid things. I do not ask about removing
codegen support for it from LLVM.

> If LLVM ever has a legitimate need for std::atomic<long long> then it
> should be usable even though it's less efficient on some targets.

You keep repeating that phrase over and over without giving a single
single reason why it is a good idea. Careless uses of 64bit atomics
*already* has been a problem in compiler-rt recently. Pretty much all
the cases I can think of for 64bit atomics on a 32bit platforms are
clear cases of significant design issues.

> Should LLVM successfully build on a target that requires libatomic to
> support std::atomic<int>? I would say 'yes', do you agree?

There are multiple implementations of atomics ops. Requiring support of
atomic operations on the native word / pointer is completely reasonable.
That doesn't necessarily mean libatomic. There is only one platform I am
aware where CAS on int can be problematic (SPARCv8 in MP configuration),
but that's about it.

> > Nothing in the change indicates that its
> > impact is limited to the libc++ test suite and.
> >
> > Joerg
>
> > Nothing in the change indicates that its impact is limited to the libc++ test suite and.
>
> (This sentence seems to stop halfway through. Am I missing part of it?)
>
> This patch potentially adds -latomic to the link command for the libLLVMSupport library but I see no problem with this for the reasons given above.

It hides use of 64bit atomic ops on common build configuration. Meaning
that any problems are like to show up much much later. It doesn't
provide anything useful for most of the LLVM ecosystem, so it is the
wrong approach. If libatomic can help testing libc++, fine, but contain
the *additional* dependency exactly to that -- the libc++ test suite.

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