[cfe-commits] r43236 - in /cfe/trunk/CodeGen: CodeGenFunction.cpp CodeGenFunction.h CodeGenTypes.cpp CodeGenTypes.h

[Chris Lattner]

> URL: http://llvm.org/viewvc/llvm-project?rev=43236&view=rev
> Log:
> Begin struct layout work.

Nice!

> --- cfe/trunk/CodeGen/CodeGenFunction.cpp (original)
> +++ cfe/trunk/CodeGen/CodeGenFunction.cpp Mon Oct 22 21:10:49 2007
> @@ -126,3 +126,14 @@
>      BB->setName(N);
>  }
>
> +/// getRecordLayoutInfo - Return record layout info.
> +RecordLayoutInfo *CodeGenFunction::getRecordLayoutInfo 
> (CodeGenTypes &CGT,
> +						       QualType RTy) {

Please remove the tabs.

> +  assert (isa<RecordType>(RTy)
> +          && "Unexpected type. RecordType expected here.");
> +
> +  const llvm::Type *Ty = ConvertType(RTy);
> +  assert (Ty && "Unable to find llvm::Type");

Can ConvertType ever return null?  If not, the assert shouldn't be  
needed.

> @@ -159,10 +167,14 @@
>        return ConvertType(cast<EnumDecl>(TD)->getIntegerType());
>      } else if (TD->getKind() == Decl::Struct) {
>        const RecordDecl *RD = cast<const RecordDecl>(TD);
> +      RecordOrganizer *RO = new RecordOrganizer();

Instead of newing and deleting RecordOrganizer, just declare it on  
the stack, like so:
         RecordOrganizer RO;

> @@ -214,3 +226,67 @@
>    }
>  }
>
> +/// getLLVMFieldNo - Return llvm::StructType element number
> +/// that corresponds to the field FD.
> +unsigned CodeGenTypes::getLLVMFieldNo(const FieldDecl *FD) {
> +  llvm::DenseMap<const FieldDecl *, unsigned>::iterator
> +    I = FieldInfo.find(FD);
> +  if (I != FieldInfo.end()  && "Unable to find field info");

This should be an assert, not an if.

> +  /// addFieldInfo - Assign field number to field FD.
> +void CodeGenTypes::addFieldInfo(const FieldDecl *FD, unsigned No) {

No spaces before the comment.

> +/// getRecordLayoutInfo - Return record layout info for the given  
> llvm::Type.
> +RecordLayoutInfo *CodeGenTypes::getRecordLayoutInfo(const  
> llvm::Type* Ty) {
> +  llvm::DenseMap<const llvm::Type*, RecordLayoutInfo *>::iterator I
> +    = RecordLayouts.find(Ty);
> +  assert (I != RecordLayouts.end()
> +          && "Unable to find record layout information for type");
> +  return I->second;

getRecordLayoutInfo should be const, and should return a const RLI*.


> +/// layoutFields - Do the actual work and lay out all fields. Create
> +/// corresponding llvm struct type.  This should be invoked only  
> after
> +/// all fields are added.
> +/// FIXME : At the moment assume
> +///    - one to one mapping between AST FieldDecls and
> +///      llvm::StructType elements.
> +///    - Ignore bit fields
> +///    - Ignore field aligments
> +///    - Ignore packed structs
> +void RecordOrganizer::layoutFields(CodeGenTypes &CGT) {
> +  // FIXME : Use SmallVector
> +  std::vector<const llvm::Type*> Fields;

Now that Fields is on the stack, just do it :)

> ====================================================================== 
> ========
> --- cfe/trunk/CodeGen/CodeGenTypes.h (original)
> +++ cfe/trunk/CodeGen/CodeGenTypes.h Mon Oct 22 21:10:49 2007
> @@ -15,6 +15,7 @@
> +
> +  /// RecordOrganizer - This helper class, used by  
> RecordLayoutInfo, layouts
> +  /// structs and unions. It manages transient information used  
> during layout.
> +  /// FIXME : At the moment assume
> +  ///    - one to one mapping between AST FieldDecls and
> +  ///      llvm::StructType elements.
> +  ///    - Ignore bit fields
> +  ///    - Ignore field aligments
> +  ///    - Ignore packed structs
> +  class RecordOrganizer {

Is it possible to move the definition of RecordOrganizer into  
CodeGenTypes.cpp?

> +  public:
> +    RecordOrganizer() : STy(NULL) {}
> +
> +    /// addField - Add new field.
> +    void addField(const FieldDecl *FD);
> +
> +    /// layoutFields - Do the actual work and lay out all fields.  
> Create
> +    /// corresponding llvm struct type.  This should be invoked  
> only after
> +    /// all fields are added.
> +    void layoutFields(CodeGenTypes &CGT);
> +
> +    /// getLLVMType - Return associated llvm struct type. This may  
> be NULL
> +    /// if fields are not laid out.
> +    llvm::Type *getLLVMType() {

This should be const.

> @@ -39,14 +89,33 @@
>    llvm::Module& TheModule;
>
>    llvm::DenseMap<const TagDecl*, llvm::Type*> TagDeclTypes;
> +
> +  /// RecordLayouts - This maps llvm struct type with corresponding
> +  /// record layout info.
> +  llvm::DenseMap<const llvm::Type*, RecordLayoutInfo *>  
> RecordLayouts;

RLI is currently only a single pointer.  Do you expect it to grow?   
If it will remain small, I'd recommend declaring this as:

> +  llvm::DenseMap<const llvm::Type*, RecordLayoutInfo> RecordLayouts;

which means that the RLI objects don't need to be explicitly new/ 
deleted.

-Chris