[cfe-dev] Objective-C: methods, ivars, and cleanup

[Devang Patel]

Hi David,

Thanks for splitting patch into separate diffs! It helps.

On Mar 24, 2008, at 11:48 AM, David Chisnall wrote:

> Hi,
>
> This is a fairly big diff, so I've tried to split it into a few  
> separate parts.
>
> globals.diff is a simple addition to the global_ctors code which  
> avoids emitting empty global_ctors.

-  // Populate the array
-  std::vector<llvm::Constant*> CtorValues;
-  llvm::Constant *MagicNumber =
-    llvm::ConstantInt::get(llvm::Type::Int32Ty, 65535, false);
-  std::vector<llvm::Constant*> StructValues;
-  for (std::vector<llvm::Constant*>::iterator I = GlobalCtors.begin(),
-       E = GlobalCtors.end(); I != E; ++I) {
-    StructValues.clear();
-    StructValues.push_back(MagicNumber);
-    StructValues.push_back(*I);
+    // Populate the array
+    std::vector<llvm::Constant*> CtorValues;
+    llvm::Constant *MagicNumber =  
llvm::ConstantInt::get(llvm::IntegerType::Int32Ty,
+        65535,
+        false);
+    for (std::vector<llvm::Constant*>::iterator I =  
GlobalCtors.begin(),
+        E = GlobalCtors.end(); I != E; ++I) {
+      std::vector<llvm::Constant*> StructValues;
+      StructValues.push_back(MagicNumber);

Hoist StructValues outside the for loop and use clear(), as it was  
done originally. Looks OK otherwise.

>
>
> etoile_runtime.diff contains a subclass of CGObjCRuntime which  
> implements the runtime-specific parts for the Étoilé runtime.  Other  
> parts do not depend on this unless output for this runtime is  
> selected (the runtime to use is currently hard-coded).  I am now  
> writing the GNU and Étoilé versions at the same time to make sure  
> the interfaces are sufficiently abstract.  Hopefully this will make  
> adding support for the Apple runtimes easy for whoever gets that job.

OK. Few nit-picks

1) Add doxygen style comments for class methods and variables. Right  
now, I don't see any comment.

2)
+#define SET(structure, index, value) do {\
+    llvm::Value *element_ptr = Builder.CreateStructGEP(structure,  
index);\
+    Builder.CreateStore(value, element_ptr);} while(0)

Avoid macros like this.

3)
+// BIG FAT WARNING: Much of this code will need factoring out later.

Just use, FIXME: Much of this code ...

4) Now, I regularly see one-parameter-per-line in your patches. For  
example,

+  // Slot type
+  SlotTy = llvm::StructType::get(IntTy,
+      IMPTy,
+      PtrToInt8Ty,
+      PtrToInt8Ty,
+      llvm::Type::Int32Ty,
+      0);

I'd avoid this myself. Clang coding style does not require this (nor  
explicitly prohibits this).

> obj_struct.diff handles conversion from Objective-C pointers to LLVM  
> types.  This is needed for accessing instance variables statically  
> and is used in the ivar access parts.  This patch is also self- 
> contained, and does not depend on any other changes.


+  void CollectIvarTypes(ObjCInterfaceDecl *ObjCClass,
+      std::vector<const llvm::Type*> *IvarTypes);

This is Objective-C specific class method, so rename it as  
CollectObjCIvarTypes.
Prefer SmallVector instead of std::vector. Pass it as a reference  
instead of a pointer.

    /// getLLVMFieldNo - Return llvm::StructType element number
    /// that corresponds to the field FD.
    unsigned getLLVMFieldNo(const FieldDecl *FD);
+  unsigned getLLVMFieldNo(QualType ObjectTy, const ObjCIvarDecl *Decl);

Why do you need new method here ?

+    // Warning: Use of this is strongly discouraged.  Late binding of  
instance
+    // variables is supported on some runtimes and so using static  
binding can
+    // break code when libraries are updated.  Only use this if you  
have
+    // previously checked that the ObjCRuntime subclass in use does  
not support
+    // late-bound ivars.

Please add assert() to check late-bound of ivars support.

> runtime_if.diff contains changes to the runtime interface.  This  
> probably needs to be committed at the same time as the changes to  
> the GNU runtime, or there might be some conflicts.  The Étoilé patch  
> may also conflict with the old definitions if it is committed before  
> this.

OK

-CGObjCRuntime *CreateObjCRuntime(llvm::Module &M);
+CGObjCRuntime *CreateObjCRuntime(llvm::Module &M,
+                                 const llvm::Type *LLVMIntType,
+                                 const llvm::Type *LLVMLongType);

Is not it possible to derive LLVMIntType and LLVMLongType info based  
on Module ?

>
> gnu_runtime.diff contains the changes made to the GNU runtime to  
> adopt the new interface.  It also adds methods for generating method  
> functions and some things related to creating class structures which  
> are not used yet.

OK. Please add doxygen style comments for each method.

>
>
> method_gen.diff contains the code required to generate Objective-C  
> methods (although it does not yet attach them to class structures.

Index: lib/CodeGen/CGStmt.cpp
===================================================================
--- lib/CodeGen/CGStmt.cpp	(revision 48734)
+++ lib/CodeGen/CGStmt.cpp	(working copy)
@@ -332,9 +332,6 @@
    // Emit the result value, even if unused, to evalute the side  
effects.
    const Expr *RV = S.getRetValue();

-  QualType FnRetTy = CurFuncDecl->getType().getCanonicalType();
-  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();
-
    if (FnRetTy->isVoidType()) {
      // If the function returns void, emit ret void.
      Builder.CreateRetVoid();

Is this intentional ?

+  // Skip the hidden arguments.
+  ++AI; ++AI;

Name two hidden arguments in the comment.

  void CodeGenFunction::GenerateCode(const FunctionDecl *FD) {
    LLVMIntTy = ConvertType(getContext().IntTy);
    LLVMPointerWidth = static_cast<unsigned>(
       
getContext 
().getTypeSize(getContext().getPointerType(getContext().VoidTy)));

    CurFuncDecl = FD;
+  FnRetTy = CurFuncDecl->getType().getCanonicalType();
+  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();
+

Is this intentional ?


>
>
> Finally, ivar.diff contains the code required for accessing and  
> modifying Objective-C instance variables.  This depends on the stuff  
> in objc_struct.diff for all runtimes that do not use late-bound  
> instance variable addresses (i.e. all of the currently supported  
> ones).

@@ -341,6 +345,17 @@
        return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD, false));
      else {
        llvm::Value *V = LocalDeclMap[D];
+      // Check if it's an implicit argument
+      if (!V) {
+        std::string VarName(D->getName());
+        llvm::Function::arg_iterator AI = CurFn->arg_begin();
+        while(AI) {
+          if(VarName == AI->getName()) {
+            return LValue::MakeAddr(AI);
+          }
+        }
+      }
        assert(V && "BlockVarDecl not entered in LocalDeclMap?");
        return LValue::MakeAddr(V);
      }

I'm getting dense here. Please explain what is "implicit argument" in  
this code? Thanks!

Overall, very nice. Thanks for continuing Objective-C code generation  
work.

-
Devang