[PATCH] [CodeGen] tighten objc ivar invariant.load attribution

[John McCall]

On Feb 14, 2013, at 9:37 AM, Saleem Abdulrasool <compnerd at compnerd.org> wrote:
> Add a few test cases and fix an inverted check.
> 
> Hi rjmccall,
> 
> http://llvm-reviews.chandlerc.com/D406
> 
> CHANGE SINCE LAST DIFF
>  http://llvm-reviews.chandlerc.com/D406?vs=976&id=982#toc
> 
> Files:
>  lib/CodeGen/CGObjCMac.cpp
>  test/CodeGenObjC/ivar-invariant-unimplemented.m
>  test/CodeGenObjC/ivar-invariant.m
> 
> Index: lib/CodeGen/CGObjCMac.cpp
> ===================================================================
> --- lib/CodeGen/CGObjCMac.cpp
> +++ lib/CodeGen/CGObjCMac.cpp
> @@ -6449,10 +6449,23 @@
>                                                unsigned CVRQualifiers) {
>   ObjCInterfaceDecl *ID = ObjectTy->getAs<ObjCObjectType>()->getInterface();
>   llvm::Value *Offset = EmitIvarOffset(CGF, ID, Ivar);
> +
> +  // Annotate the load as an invariant load iff the object type is the type, or
> +  // a derived type, of the class containing the ivar within an ObjC method.
> +  // This check is needed because the ivar offset is a lazily initialised value
> +  // that may depend on objc_msgSend to perform a fixup on the first message
> +  // dispatch.
> +  //
> +  // An additional opportunity to mark the load as invariant arises when the
> +  // base of the ivar access is a parameter to an Objective C method.  However,
> +  // because the parameters are not available in the current interface, we
> +  // cannot perform this check.

Go ahead and tweak the interface to propagate through an optional
base expression and then implement your check based on that.

>   if (llvm::LoadInst *LI = dyn_cast<llvm::LoadInst>(Offset))

I know this is old code, but you can just use cast<llvm::LoadInst> here;
the dyn_cast is not necessary.

> -    LI->setMetadata(CGM.getModule().getMDKindID("invariant.load"), 
> -                   llvm::MDNode::get(VMContext,
> -                   ArrayRef<llvm::Value*>()));
> +    if (dyn_cast<ObjCMethodDecl>(CGF.CurFuncDecl))
> +      if (Ivar->getContainingInterface()->isSuperClassOf(ID))

Please extract this condition out into a separate function.

> +        LI->setMetadata(CGM.getModule().getMDKindID("invariant.load"),
> +                        llvm::MDNode::get(VMContext, ArrayRef<llvm::Value*>()));
> +
>   return EmitValueForIvarAtOffset(CGF, ID, BaseValue, Ivar, CVRQualifiers,
>                                   Offset);
> }
> Index: test/CodeGenObjC/ivar-invariant-unimplemented.m
> ===================================================================
> --- /dev/null
> +++ test/CodeGenObjC/ivar-invariant-unimplemented.m
> @@ -0,0 +1,32 @@
> +// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm -x objective-c %s -o - | FileCheck %s
> +// XFAIL: *

We usually don't add xfailed tests for missing optimizations and features.
The proper way to keep track of this is in the bug tracker, not the test suite.

> Index: test/CodeGenObjC/ivar-invariant.m
> ===================================================================
> --- /dev/null
> +++ test/CodeGenObjC/ivar-invariant.m
> @@ -0,0 +1,55 @@
> +// RUN: %clang_cc1 -triple x86_64-apple-darwin -x objective-c %s -o - | FileCheck %s
> +
> + at interface NSObject
> ++ (id) new;
> +- (id) init;
> + at end
> +
> + at interface Base : NSObject @end
> +
> +// @implementation Base
> +// {
> +//     int dummy;
> +// }
> +// @end
> +
> + at interface Derived : Base
> +{
> +    @public int member;
> +}
> + at end
> +
> + at implementation Derived
> +- (id) init
> +{
> +    self = [super init];
> +    member = 42;
> +    return self;
> +}
> + at end
> +
> +// CHECK: %ivar = load i64* @"OBJC_IVAR_$_Derived.member", !invariant.load

You can't test for LLVM IR names like this, because we strip names in non-asserts
builds.  If you actually needed the name for later test lines, you would need to match
a pattern against it, like so:
  [[IVAR:%.*]] = load ...
In this case, you don't, so you're fine.

> +
> +void * variant_load_1(int i) {
> +    void *ptr;
> +    while (i--) {
> +        Derived *d = [Derived new];
> +        ptr = &d->member;
> +    }
> +    return ptr;
> +}
> +
> +// CHECK: %ivar = load i64* @"OBJC_IVAR_$_Derived.member"
> +// CHECK-NOT: %ivar = load i64* @"OBJC_IVAR_$_Derived.member", !invariant.load

This is not how CHECK-NOT works.  What you actually want to do is
  //  CHECK: %ivar = load i64* @"OBJC_IVAR_$_Derived.member"{{$}}

John.