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

[Devang Patel]

On Oct 23, 2007, at 4:58 PM, Chris Lattner wrote:

>> 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.

ok

>
>
>> +  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?

no

> If not, the assert shouldn't be needed.

ok
>
>
>> @@ -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;

ok

>> @@ -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.

who did this snick in ? :)
>
>
>> +  /// addFieldInfo - Assign field number to field FD.
>> +void CodeGenTypes::addFieldInfo(const FieldDecl *FD, unsigned No) {
>
> No spaces before the comment.

ok

>
>
>> +/// 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*.

ok

>
>
>
>> +/// 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 :)

yeah

>
>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- 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?

possible. I'll do it.
>
>
>> +  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.

ok

>> @@ -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?

Yes. It will have special info for fields that requires masking during  
load and store.

>  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
>

-
Devang