Some fixes to the Objective-C optimisation passes. Tested by compiling GNUstep base with clang at -O3 + all of the optimisations that this library enables at O3 and then running the test suite - results, looking good. Still to do before the release: turn objc_msgSend() into objc_msg_lookup_sender() and insert caching when not optimising for size.

opts/IMPCacher.cpp

@@ -40,21 +40,39 @@ void GNUstep::IMPCacher::CacheLookup(Instruction *lookup, Value *slot, Value
 
   removeTerminator(beforeLookupBB);
 
-  IRBuilder<> B = IRBuilder<>(beforeLookupBB);
+  CGBuilder B = CGBuilder(beforeLookupBB);
   // Load the slot and check that neither it nor the version is 0.
-  Value *slotValue = B.CreateLoad(slot);
   Value *versionValue = B.CreateLoad(version);
   Value *receiverPtr = lookup->getOperand(0);
   Value *receiver = receiverPtr;
   if (!isSuperMessage) {
     receiver = B.CreateLoad(receiverPtr);
   }
+  // For small objects, we skip the cache entirely.  
+  // FIXME: Class messages are never to small objects...
+  bool is64Bit = llvm::Module::Pointer64 ==
+    B.GetInsertBlock()->getParent()->getParent()->getPointerSize();
+  LLVMType *intPtrTy = is64Bit ? Type::getInt64Ty(Context) :
+    Type::getInt32Ty(Context);
+
+  // Receiver as an integer
+  Value *receiverSmallObject = B.CreatePtrToInt(receiver, intPtrTy);
+  // Receiver is a small object...
+  receiverSmallObject =
+    B.CreateAnd(receiverSmallObject, is64Bit ? 7 : 1);
+  // Receiver is not a small object.
+  receiverSmallObject =
+    B.CreateICmpNE(receiverSmallObject, Constant::getNullValue(intPtrTy));
+  // Ideally, we'd call objc_msgSend() here, but for now just skip the cache
+  // lookup
 
   Value *isCacheEmpty = 
     B.CreateICmpEQ(versionValue, Constant::getNullValue(IntTy));
-  Value *receiverNotNil =
+  Value *receiverNil =
-     B.CreateICmpNE(receiver, Constant::getNullValue(receiver->getType()));
+     B.CreateICmpEQ(receiver, Constant::getNullValue(receiver->getType()));
-  isCacheEmpty = B.CreateAnd(isCacheEmpty, receiverNotNil);
+
+  isCacheEmpty = B.CreateOr(isCacheEmpty, receiverNil);
+  isCacheEmpty = B.CreateOr(isCacheEmpty, receiverSmallObject);
       
   BasicBlock *cacheLookupBB = BasicBlock::Create(Context, "cache_check",
       lookupBB->getParent());
@@ -63,6 +81,7 @@ void GNUstep::IMPCacher::CacheLookup(Instruction *lookup, Value *slot, Value
 
   // Check the cache node is current
   B.SetInsertPoint(cacheLookupBB);
+  Value *slotValue = B.CreateLoad(slot, "slot_value");
   Value *slotVersion = B.CreateStructGEP(slotValue, 3);
   // Note: Volatile load because the slot version might have changed in
   // another thread.
@@ -76,14 +95,15 @@ void GNUstep::IMPCacher::CacheLookup(Instruction *lookup, Value *slot, Value
   // If this slot is still valid, skip the lookup.
   B.CreateCondBr(isSlotValid, afterLookupBB, lookupBB);
 
+  // Perform the real lookup and cache the result
+  removeTerminator(lookupBB);
   // Replace the looked up slot with the loaded one
   B.SetInsertPoint(afterLookupBB, afterLookupBB->begin());
+  PHINode *newLookup = IRBuilderCreatePHI(&B, lookup->getType(), 3, "new_lookup");
   // Not volatile, so a redundant load elimination pass can do some phi
   // magic with this later.
-  lookup->replaceAllUsesWith(B.CreateLoad(slot));
+  lookup->replaceAllUsesWith(newLookup);
 
-  // Perform the real lookup and cache the result
-  removeTerminator(lookupBB);
   B.SetInsertPoint(lookupBB);
   Value * newReceiver = receiver;
   if (!isSuperMessage) {
@@ -93,18 +113,25 @@ void GNUstep::IMPCacher::CacheLookup(Instruction *lookup, Value *slot, Value
       lookupBB->getParent());
 
   // Don't store the cached lookup if we are doing forwarding tricks.
-  B.CreateCondBr(B.CreateICmpEQ(receiver, newReceiver), storeCacheBB,
+  // Also skip caching small object messages for now
-      afterLookupBB);
+  Value *skipCacheWrite =
+    B.CreateOr(B.CreateICmpNE(receiver, newReceiver), receiverSmallObject);
+  skipCacheWrite = B.CreateOr(skipCacheWrite, receiverNil);
+  B.CreateCondBr(skipCacheWrite, afterLookupBB, storeCacheBB);
   B.SetInsertPoint(storeCacheBB);
 
   // Store it even if the version is 0, because we always check that the
   // version is not 0 at the start and an occasional redundant store is
   // probably better than a branch every time.
   B.CreateStore(lookup, slot);
-  B.CreateStore(B.CreateLoad(B.CreateStructGEP(lookup, 3)), version);
+  //B.CreateStore(B.CreateLoad(B.CreateStructGEP(lookup, 3)), version);
   cls = B.CreateLoad(B.CreateBitCast(receiver, IdTy));
   B.CreateStore(cls, B.CreateStructGEP(lookup, 1));
   B.CreateBr(afterLookupBB);
+
+  newLookup->addIncoming(lookup, lookupBB);
+  newLookup->addIncoming(slotValue, cacheLookupBB);
+  newLookup->addIncoming(lookup, storeCacheBB);
 }
 
 

opts/IvarPass.cpp

@@ -7,14 +7,26 @@
 #include "llvm/Constants.h"
 #include "llvm/Analysis/Verifier.h"
 #include "llvm/DefaultPasses.h"
+#include "llvm/ADT/DenseSet.h"
 #include "ObjectiveCOpts.h"
 #include <string>
 
 using namespace llvm;
 using std::string;
 
+typedef std::pair<Instruction*, Value*> Replacement;
+
+namespace llvm {
+template<> struct DenseMapInfo<Replacement> {
+  static inline Replacement getEmptyKey() { return Replacement(0,0); }
+  static inline Replacement getTombstoneKey() { return Replacement(0, (Value*)-1); }
+  static unsigned getHashValue(const Replacement& Val) { return ((uintptr_t)Val.first) * 37U; }
+  static bool isEqual(const Replacement& LHS, const Replacement& RHS) {
+    return LHS.first == RHS.first;
+  }
+};
+}
 namespace {
-
   class GNUNonfragileIvarPass : public FunctionPass {
 
     public:
@@ -88,8 +100,7 @@ namespace {
 
     virtual bool runOnFunction(Function &F) {
       bool modified = false;
-      typedef std::pair<Instruction*, Value*> Replacement;
+      llvm::DenseSet<Replacement> replacements;
-      llvm::SmallVector<Replacement, 16> replacements;
       //llvm::cerr << "IvarPass: " << F.getName() << "\n";
       for (Function::iterator i=F.begin(), end=F.end() ;
           i != end ; ++i) {
@@ -115,16 +126,16 @@ namespace {
                 // If the class, and all superclasses, are visible in this module
                 // then we can hard-code the ivar offset
                 if (size_t offset = hardCodedOffset(className, ivarName)) {
-                  replacements.push_back(Replacement(load, 0));
+                  replacements.insert(Replacement(indirectload,
-                  replacements.push_back(Replacement(indirectload,
                               ConstantInt::get(indirectload->getType(), offset)));
+                  replacements.insert(Replacement(load, 0));
                   modified = true;
                 } else {
                   // If the class was compiled with the new ABI, then we have a
                   // direct offset variable that we can use
                   if (Value *offset = M->getGlobalVariable(
                               ("__objc_ivar_offset_value_" + suffix).str())) {
-                    replacements.push_back(Replacement(load, offset));
+                    replacements.insert(Replacement(load, offset));
                     modified = true;
                   }
                 }
@@ -133,7 +144,7 @@ namespace {
           }
         }
       }
-      for (SmallVector<Replacement, 16>::iterator i=replacements.begin(),
+      for (DenseSet<Replacement>::iterator i=replacements.begin(),
               end=replacements.end() ; i != end ; ++i) {
         if (i->second) 
           i->first->replaceAllUsesWith(i->second);

opts/LoopIMPCachePass.cpp

@@ -78,7 +78,7 @@ namespace
         cacher->CacheLookup(*i, slot, version);
       }
       if (modified){
-          verifyFunction(F);
+        verifyFunction(F);
       }
       return modified;
     }