[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 8:34 PM, John McCall <rjmccall at apple.com> wrote:
> Author: rjmccall
> Date: Mon Sep 12 22:34:09 2011
> New Revision: 139580
>
> URL: http://llvm.org/viewvc/llvm-project?rev=139580&view=rev
> Log:
> Unify the decision of how to emit property getters and setters into a
> single code path.  Use atomic loads and stores where necessary.  Load and
> store anything of the appropriate size and alignment with primitive
> operations instead of going through the call.
>
>
> Modified:
>    cfe/trunk/lib/CodeGen/CGObjC.cpp
>    cfe/trunk/lib/CodeGen/CodeGenFunction.h
>    cfe/trunk/test/CodeGenObjC/atomic-aggregate-property.m
>
> Modified: cfe/trunk/lib/CodeGen/CGObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?rev=139580&r1=139579&r2=139580&view=diff
> ==============================================================================
> --- cfe/trunk/lib/CodeGen/CGObjC.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGObjC.cpp Mon Sep 12 22:34:09 2011
> @@ -374,152 +374,325 @@
>                fn, ReturnValueSlot(), args);
>  }
>
> -// 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.

> +/// 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).

> +  // Otherwise, this is target-dependent and based on the size and
> +  // alignment of the ivar.
> +  llvm::Triple::ArchType arch =
> +    CGM.getContext().getTargetInfo().getTriple().getArch();
> +
> +  // Most architectures require memory to fit within a single cache
> +  // line, so the alignment has to be at least the size of the access.
> +  // Otherwise we have to grab a lock.
> +  if (IvarAlignment < IvarSize && !hasUnalignedAtomics(arch)) {
> +    Kind = CopyStruct;
> +    return;
> +  }
> +
> +  // If the ivar's size exceeds the architecture's maximum atomic
> +  // access size, we have to use CopyStruct.
> +  if (IvarSize > getMaxAtomicAccessSize(CGM, arch)) {
> +    Kind = CopyStruct;
> +    return;
> +  }
> +
> +  // Otherwise, we can use native loads and stores.
> +  Kind = Native;
> +}

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.

>  /// GenerateObjCGetter - Generate an Objective-C property getter
>  /// function. The given Decl must be an ObjCImplementationDecl. @synthesize
>  /// is illegal within a category.
>  void CodeGenFunction::GenerateObjCGetter(ObjCImplementationDecl *IMP,
>                                          const ObjCPropertyImplDecl *PID) {
> -  ObjCIvarDecl *Ivar = PID->getPropertyIvarDecl();
>   const ObjCPropertyDecl *PD = PID->getPropertyDecl();
> -  bool IsAtomic =
> -    !(PD->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_nonatomic);
>   ObjCMethodDecl *OMD = PD->getGetterMethodDecl();
>   assert(OMD && "Invalid call to generate getter (empty method)");
>   StartObjCMethod(OMD, IMP->getClassInterface(), PID->getLocStart());
> -
> -  // Determine if we should use an objc_getProperty call for
> -  // this. Non-atomic properties are directly evaluated.
> -  // atomic 'copy' and 'retain' properties are also directly
> -  // evaluated in gc-only mode.
> -  if (CGM.getLangOptions().getGCMode() != LangOptions::GCOnly &&
> -      IsAtomic &&
> -      (PD->getSetterKind() == ObjCPropertyDecl::Copy ||
> -       PD->getSetterKind() == ObjCPropertyDecl::Retain)) {
> -    llvm::Value *GetPropertyFn =
> -      CGM.getObjCRuntime().GetPropertyGetFunction();
>
> -    if (!GetPropertyFn) {
> -      CGM.ErrorUnsupported(PID, "Obj-C getter requiring atomic copy");
> -      FinishFunction();
> +  generateObjCGetterBody(IMP, PID);
> +
> +  FinishFunction();
> +}
> +
> +static bool hasTrivialGetExpr(const ObjCPropertyImplDecl *PID) {
> +  const Expr *getter = PID->getGetterCXXConstructor();
> +  if (!getter) return true;
> +
> +  // Sema only makes only of these when the ivar has a C++ class type,
> +  // so the form is pretty constrained.
> +
> +  // If we selected a trivial copy-constructor, we're okay.
> +  if (const CXXConstructExpr *construct = dyn_cast<CXXConstructExpr>(getter))
> +    return (construct->getConstructor()->isTrivial());
> +
> +  // The constructor might require cleanups (in which case it's never
> +  // trivial).
> +  assert(isa<ExprWithCleanups>(getter));
> +  return false;
> +}
> +
> +void
> +CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl *classImpl,
> +                                        const ObjCPropertyImplDecl *propImpl) {
> +  // If there's a non-trivial 'get' expression, we just have to emit that.
> +  if (!hasTrivialGetExpr(propImpl)) {
> +    ReturnStmt ret(SourceLocation(), propImpl->getGetterCXXConstructor(),
> +                   /*nrvo*/ 0);
> +    EmitReturnStmt(ret);
> +    return;
> +  }
> +
> +  const ObjCPropertyDecl *prop = propImpl->getPropertyDecl();
> +  QualType propType = prop->getType();
> +  ObjCMethodDecl *getterMethod = prop->getGetterMethodDecl();
> +
> +  ObjCIvarDecl *ivar = propImpl->getPropertyIvarDecl();
> +
> +  // Pick an implementation strategy.
> +  PropertyImplStrategy strategy(CGM, propImpl);
> +  switch (strategy.getKind()) {
> +  case PropertyImplStrategy::Native: {
> +    LValue LV = EmitLValueForIvar(TypeOfSelfObject(), LoadObjCSelf(), ivar, 0);
> +
> +    // Currently, all atomic accesses have to be through integer
> +    // types, so there's no point in trying to pick a prettier type.
> +    llvm::Type *bitcastType =
> +      llvm::Type::getIntNTy(getLLVMContext(),
> +                            getContext().toBits(strategy.getIvarSize()));
> +    bitcastType = bitcastType->getPointerTo(); // addrspace 0 okay
> +
> +    // Perform an atomic load.  This does not impose ordering constraints.
> +    llvm::Value *ivarAddr = LV.getAddress();
> +    ivarAddr = Builder.CreateBitCast(ivarAddr, bitcastType);
> +    llvm::LoadInst *load = Builder.CreateLoad(ivarAddr, "load");
> +    load->setAlignment(strategy.getIvarAlignment().getQuantity());
> +    load->setAtomic(llvm::Unordered);

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?

-Eli