[cfe-commits] r139580 - in /cfe/trunk: lib/CodeGen/CGObjC.cpp lib/CodeGen/CodeGenFunction.h test/CodeGenObjC/atomic-aggregate-property.m

[Eli Friedman]

On Mon, Sep 12, 2011 at 11:16 PM, John McCall <rjmccall at apple.com> wrote:
> On Sep 12, 2011, at 10:07 PM, Eli Friedman wrote:
>> On Mon, Sep 12, 2011 at 8:34 PM, John McCall <rjmccall at apple.com> wrote:
>>> -// FIXME: I wasn't sure about the synthesis approach. If we end up generating an
>>> -// AST for the whole body we can just fall back to having a GenerateFunction
>>> -// which takes the body Stmt.
>>> +/// Determine whether the given architecture supports unaligned atomic
>>> +/// accesses.  They don't have to be fast, just faster than a function
>>> +/// call and a mutex.
>>> +static bool hasUnalignedAtomics(llvm::Triple::ArchType arch) {
>>> +  return (arch == llvm::Triple::x86 || arch == llvm::Triple::x86_64);
>>> +}
>>
>> Hmm... I'm pretty sure we'll end up generating wrong code for this at
>> the moment.  I can fix that pretty easily, though.
>
> If you don't do it soon-ish, let me know and I'll put it back.

K.

>>> +/// Return the maximum size that permits atomic accesses for the given
>>> +/// architecture.
>>> +static CharUnits getMaxAtomicAccessSize(CodeGenModule &CGM,
>>> +                                        llvm::Triple::ArchType arch) {
>>> +  // ARM has 8-byte atomic accesses, but it's not clear whether we
>>> +  // want to rely on them here.
>>
>> I don't see any problem with relying on them... but the compiler
>> probably wouldn't end up using them very often. Nothing is normally
>> aligned to 8 bytes on ARM (well, at least not on iOS).
>
> Er.  Clang, at least, seems to evaluate __alignof(double) as 8, and
> vector types are probably the same.

IIRC, that isn't the ABI alignment.

> I mostly just wanted to check things over with the runtime people
> before flipping this particular switch.

K.

>> I think you're missing a check on the size here: we do not currently
>> support i24 atomic stores, and I do not intend to implement such
>> support.
>
> Good catch!  Anything subtle besides "power of two"?  Are there
> architectures that don't guarantee atomicity of stores *smaller* than
> pointer size?

I don't know of any architecture that supports atomic stores of
pointer size, and has smaller stores which are not atomic.  There are
architectures which do not have any stores smaller than a pointer...
LLVM does not support any such architecture, though, and I doubt
anyone would try to use Objective-C on such an architecture.

>> I want to double-check that Unordered is really what you want... if a
>> value starts off at 0, one thread sets it to 1, and another thread
>> calls the getter twice, is it legal if the first getter call returns
>> 1, and the second 0?
>
> Probably not;  I wasn't considering that level of causality breakage.
> I'll talk it over with the runtime folks;  we probably want this to be
> release/acquire or sequentially consistent or something.

SequentiallyConsistent will get you the same behavior as an
implementation with locks, and is the only way to completely avoid
causality violations with atomic properties. That would be the most
conservative choice, but I can't really say whether that's the right
choice without knowing how people write multi-threaded Objective-C
code.

-Eli