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

[Daniel Sanders via llvm-commits]

Hi Jeorg,

Please keep me on the recipients list.

> -----Original Message-----
> From: llvm-commits [mailto:llvm-commits-bounces at lists.llvm.org] On Behalf
> Of Joerg Sonnenberger via llvm-commits
> 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.

Stupid or not, we don't meet the standard if we don't allow the things it requires.

There is no CodeGen support for i64 atomics on this 32-bit target.

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

I do not think it's a good idea to have 64-bit atomics in LLVM.

All I've been saying is that std::atomic<long long> should work when a programmer uses it and that it's ok for the implementation of this to be slow if faster implementations are impossible. Maybe I've misunderstood your earlier emails but you appeared to be arguing that it shouldn't be allowed to work at all on the basis that a working implementation would be slower than std::atomic<int>.

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

Yes, there are multiple implementations of atomic ops. This question was trying to establish whether you think it has to be implementation provided by the native hardware or not. I don't think it does, and that any implementation that meets the requirements of the standard is good enough. Without this, it becomes impossible to build LLVM on a target that lacks something like load-linked/store-conditional.

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

If the emulation meets the requirements then there shouldn't be any problems, and enforcing voluntary restrictions sounds like a job for something like clang-tidy to me. Still, I have no problem with keeping this to libcxx (although I'm not keen on the code duplication required).