>>> + assert(!verifyFunction(*CurFn)); >> >> nit-pick, add a message here. > > That code was copied-and-pasted from the GenerateCode method used > for generating functions. For completeness I've added a message in > to where I originally copied it from as well.
Hi David, Some more minor nit-picky stuff (this is why small patches are better than big ones ;-): +++ lib/CodeGen/CGObjCGNU.cpp (working copy) +#include <stdarg.h> Use <cstdarg> and move it to the end of the #include list if it is needed. + llvm::Value *GenerateIvarList(llvm::LLVMFoldingBuilder &Builder, + std::vector<llvm::Constant*> MethodNames, + std::vector<llvm::Constant*> MethodTypes, + std::vector<llvm::Constant*> MethodIMPs); You're passing these vectors by value, I'd suggest passing them by (const) reference unless you really really want a copy made. likewise in a few other places. + // Get the selector Type. + std::vector<const llvm::Type*> Str2(2, PtrToInt8Ty); + const llvm::Type *SelStructTy = llvm::StructType::get(Str2); Please use the variadic version of STructType::get to avoid the vector. + // C string type. Used in lots of places. + PtrToInt8Ty = + llvm::PointerType::getUnqual(llvm::Type::Int8Ty); + // Get the selector Type. + std::vector<const llvm::Type*> Str2(2, PtrToInt8Ty); + const llvm::Type *SelStructTy = llvm::StructType::get(Str2); + SelectorTy = llvm::PointerType::getUnqual(SelStructTy); + PtrToIntTy = llvm::PointerType::getUnqual(IntTy); + PtrTy = llvm::PointerType::getUnqual(llvm::Type::Int8Ty); You call PointerType::getUnqual(llvm::Type::Int8Ty) twice here, please eliminate one. + // Object type + llvm::OpaqueType *OpaqueObjTy = llvm::OpaqueType::get(); + llvm::Type *OpaqueIdTy = llvm::PointerType::getUnqual(OpaqueObjTy); + IdTy = llvm::PointerType::getUnqual(llvm::StructType::get(OpaqueIdTy, NULL)); + OpaqueObjTy->refineAbstractTypeTo(IdTy); This code is unsafe, you need to use a PATypeHolder as described here: http://llvm.org/docs/ProgrammersManual.html#TypeResolve Likewise in GenerateMethodList and other places you use refineAbstractTypeTo +static void SetField(llvm::LLVMFoldingBuilder &Builder, + llvm::Value *Structure, + unsigned Index, + llvm::Value *Value) { + llvm::Value *element_ptr = Builder.CreateStructGEP(Structure, Index); + Builder.CreateStore(Value, element_ptr); Nice, much better than a macro ;-) ;-). Please only indent the body by 2, not 4 though. Also, you could just use: + Builder.CreateStore(Value, Builder.CreateStructGEP(Structure, Index)); at which point it might make more sense to just inline this everywhere it is used. + if(SelTypes == 0) { + llvm::Constant *SelFunction = + TheModule.getOrInsertFunction("sel_get_uid", SelectorTy, PtrToInt8Ty, NULL); + cmd = Builder.CreateCall(SelFunction, SelName); 80 cols. + llvm::SmallVector<llvm::Value*, 2> Args; + Args.push_back(SelName); + Args.push_back(SelTypes); + cmd = Builder.CreateCall(SelFunction, Args.begin(), Args.end()); There is no need to use a SmallVector when the size is fixed, I'd just use: llvm::Value *Args[] = { SelName, SelTypes }; cmd = Builder.CreateCall(SelFunction, Args, Args+2); + for(unsigned int i=0 ; i<1 ; i++) { urr? A single iteration loop? + std::vector<const llvm::Type*> Args; + Args.push_back(SelfTy); + Args.push_back(SelectorTy); + for (unsigned i=0; i<ArgC ; i++) { + Args.push_back(ArgTy[i]); + } Instead of the loop, you can use: + Args.insert(Args.end(), ArgTy, ArgTy+ArgC); + llvm::FunctionType *MethodTy = llvm::FunctionType::get(ReturnTy, Args, isVarArg); 80 cols + FnRetTy = OMD->getResultType(); ///.getCanonicalType(); Please remove this comment or improve it. + FnRetTy = CurFuncDecl->getType().getCanonicalType(); + FnRetTy = cast<FunctionType>(FnRetTy)->getResultType(); This would be more idiomatic to write as: FnRetTy = CurFuncDecl->getType()->getAsFunctionType()->getResultType(); +Value *ScalarExprEmitter::VisitObjCIvarRefExpr(ObjCIvarRefExpr *E) { A lot of this code will be independent of whether the IVar is a scalar, complex or aggregate. I'd suggest moving the "get the address of an ivar" piece into CGObjC*.cpp and just have VisitObjCIvarRefExpr be a simple function which calls into the "form address" part and then just does a load. This can be done as a follow on patch, but will be required to get Complex and aggregate (e.g. struct) ivars working. ... actually, isn't this function just "EmitObjCIvarRefLValue" ? @@ -341,6 +345,17 @@ return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD, false)); else { llvm::Value *V = LocalDeclMap[D]; + // Check if it's an implicit argument + // TODO: Other runtimes may set non-argument hidden variables + if (!V) { + std::string VarName(D->getName()); + llvm::Function::arg_iterator AI = CurFn->arg_begin(); + do { + if(VarName == AI->getName()) { + return LValue::MakeAddr(AI); + } + } while(++AI); + } assert(V && "BlockVarDecl not entered in LocalDeclMap?"); This is incredibly inefficient, what are you trying to accomplish here? I'd suggest removing this and handling it as part of a follow- on patch. -Chris _______________________________________________ cfe-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
