+ 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.
Ooops. Left-over from a thing that isn't used anymore. Deleted.
+ 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.
I'm not using them at all yet, so feel free not to commit them yet... (Fixed, in the meantime)
+ // 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.
Fixed.
+ // 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.
Fixed.
+ // 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#TypeResolveLikewise in GenerateMethodList and other places you use
refineAbstractTypeTo
I think I've got all of them - let me know if I missed any. (Refining abstract types is now my answer to your earlier question of what can be done cleanly with a macro but not with a static function).
+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.
Fixed.
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.
Probably true. I'll consider making this change in the next revision.
+ if(SelTypes == 0) {
+ llvm::Constant *SelFunction =
+ TheModule.getOrInsertFunction("sel_get_uid", SelectorTy,
PtrToInt8Ty, NULL);
+ cmd = Builder.CreateCall(SelFunction, SelName);80 cols.
Fixed.
+ 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);
Fixed.
+ for(unsigned int i=0 ; i<1 ; i++) {
urr? A single iteration loop?
Ooops. That was a placeholder. That method and its companion are untested. My next task is finishing class generation, at which point I can test them properly (and, hopefully, I will then be able to compile and link simple Objective-C programs and start writing some proper tests rather than just looking at the IR).
+ 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);
Thanks. I'm still trying to remember how STL works...
+ llvm::FunctionType *MethodTy = llvm::FunctionType::get(ReturnTy,
Args, isVarArg);80 cols
Fixed. And in a few other places I missed earlier.
+ FnRetTy = OMD->getResultType(); ///.getCanonicalType();
Please remove this comment or improve it.
Ooops, left-over from copying and pasting.
+ FnRetTy = CurFuncDecl->getType().getCanonicalType();
+ FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();This would be more idiomatic to write as:
FnRetTy = CurFuncDecl->getType()->getAsFunctionType()->getResultType();
Fixed. (This one is someone else's code - I just moved it, I didn't write it).
+Value *ScalarExprEmitter::VisitObjCIvarRefExpr(ObjCIvarRefExpr *E) {
[Aren't you doing something stupid there] (parahprased)
Yes. Yes I am. Fixed, I think.
@@ -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-
another Objective-C runtime that uses more than two, which is possible without changing any of the non-runtime-specific code, but unlikely to happen). In most cases it won't run at all, since the LocalDeclMap will give you the correct value. If it doesn't, then it means that you've referred to self or _cmd (or self or _call in the Étoilé runtime). In this case, then there is no decl for the argument (and it doesn't seem to be possible to easily create one from anything other than the parser), and so it checks the implicit arguments. If you can add a constructor that lets a ParamDecl be created easily from the code generator then I'd be happy to use that instead (although I'm not 100% sure that would make sense, semantically). As always, suggestions welcome...
Thanks for the feedback,
David
clang.diff (45.2 KB)