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

[Saleem Abdulrasool]

On Thu, Feb 14, 2013 at 11:23 AM, John McCall <rjmccall at apple.com> wrote:
> 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.

Do you mind if I were to do that as a follow on change?

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

Done.

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

Done.

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

Fair enough.  Removed.

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

Changed anyways.

>> +
>> +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"{{$}}

Thanks, changed.

> John.



-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org