[llvm-commits] [llvm] r89176 - in /llvm/trunk: include/llvm/Target/TargetData.h lib/Target/TargetData.cpp

[Chris Lattner]

On Nov 17, 2009, at 5:03 PM, Bill Wendling wrote:

> Author: void
> Date: Tue Nov 17 19:03:56 2009
> New Revision: 89176
>
> URL: http://llvm.org/viewvc/llvm-project?rev=89176&view=rev
> Log:
> The llvm-gcc front-end and the pass manager use two separate  
> TargetData objects.
> This is probably not confined to *just* these two things.

Hi Bill,

This commit message doesn't really make sense to me at all.  I would  
have expected it to be something like "Convert TargetData to use an  
AbstractTypesUser so that it doesn't have dangling pointers when  
abstract types get resolved".


> +++ llvm/trunk/include/llvm/Target/TargetData.h Tue Nov 17 19:03:56  
> 2009
> @@ -30,6 +30,7 @@
> class IntegerType;
> class StructType;
> class StructLayout;
> +class StructLayoutMap;

Why did you publish the StructLayoutMap type?  If you keep it a void*,  
you can put it in an anonymous namespace in the .cpp file.

> +++ llvm/trunk/lib/Target/TargetData.cpp Tue Nov 17 19:03:56 2009
> @@ -323,37 +323,130 @@
>                  : Alignments[BestMatchIdx].PrefAlign;
> }
>
> -typedef DenseMap<const StructType*, StructLayout*>LayoutInfoTy;
> +typedef DenseMap<const StructType*, StructLayout*> LayoutInfoTy;
>
> +namespace llvm {
> +
> +class StructLayoutMap : public AbstractTypeUser {
> +  LayoutInfoTy LayoutInfo;
> +
> +  /// refineAbstractType - The callback method invoked when an  
> abstract type is
> +  /// resolved to another type.  An object must override this  
> method to update
> +  /// its internal state to reference NewType instead of OldType.
> +  ///
> +  virtual void refineAbstractType(const DerivedType *OldTy,
> +                                  const Type *) {
> +    const StructType *STy = dyn_cast<const StructType>(OldTy);
> +    if (!STy) {
> +      OldTy->removeAbstractTypeUser(this);
> +      return;

This should be an error, so you should assert on "STy != 0".  The only  
types this AbstractTypeUser tracks are structs.

> +    }
> +
> +    StructLayout *SL = LayoutInfo[STy];
> +    if (SL) {
> +      SL->~StructLayout();
> +      free(SL);
> +      LayoutInfo[STy] = NULL;

This should use find() instead of [] for two reasons.

1. STy *must* be in the densemap, so this should be an assert that the  
iterator != end.
2. You don't want to set the entry for STy to null, you want to delete  
the entry in the Densemap with erase(iterator).

> +  /// typeBecameConcrete - The other case which AbstractTypeUsers  
> must be aware
> +  /// of is when a type makes the transition from being abstract  
> (where it has
> +  /// clients on its AbstractTypeUsers list) to concrete (where it  
> does not).
> +  /// This method notifies ATU's when this occurs for a type.
> +  ///
> +  virtual void typeBecameConcrete(const DerivedType *AbsTy) {

Same comments as refineAbstractType.

> +  bool insert(const Type *Ty) {
> +    if (Ty->isAbstract())
> +      Ty->addAbstractTypeUser(this);
> +    return true;
> +  }

Just inline this into its one caller.


> +public:
> +  virtual ~StructLayoutMap() {
> +    // Remove any layouts.
> +    for (LayoutInfoTy::iterator
> +           I = LayoutInfo.begin(), E = LayoutInfo.end(); I != E; ++I)
> +      if (StructLayout *SL = I->second) {
> +        SL->~StructLayout();
> +        free(SL);
> +      }

This needs to unregister as listeners from the abstract types!

> +  }
> +
> +  inline LayoutInfoTy::iterator begin() {
> +    return LayoutInfo.begin();
> +  }
> +  inline LayoutInfoTy::iterator end() {
> +    return LayoutInfo.end();
> +  }
> +  inline LayoutInfoTy::const_iterator begin() const {
> +    return LayoutInfo.begin();
> +  }
> +  inline LayoutInfoTy::const_iterator end() const {
> +    return LayoutInfo.end();
> +  }
> +
> +  LayoutInfoTy::iterator find(const StructType *&Val) {
> +    return LayoutInfo.find(Val);
> +  }
> +  LayoutInfoTy::const_iterator find(const StructType *&Val) const {
> +    return LayoutInfo.find(Val);
> +  }
> +
> +  bool erase(const StructType *&Val) {
> +    return LayoutInfo.erase(Val);
> +  }
> +  bool erase(LayoutInfoTy::iterator I) {
> +    return LayoutInfo.erase(I);
> +  }

This stuff is all dead.  Please remove it.

> +
> +  StructLayout *&operator[](const Type *Key) {

Key should be declared as 'const StructType *'.

> +    const StructType *STy = dyn_cast<const StructType>(Key);
> +    assert(STy && "Trying to access the struct layout map with a  
> non-struct!");
which allows you to eliminate this.

> +    insert(STy);
> +    return LayoutInfo[STy];

Don't do a redundant map lookup after 'insert'.

-Chris