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

[Bill Wendling]

On Dec 1, 2009, at 2:15 PM, Chris Lattner wrote:

> 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".
>
Ah. Sorry about that.

>> +++ 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.
>
Because I don't like casting to and from void*. :-) But sure.

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

>> +    }
>> +
>> +    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.
>
Well, not all of them. :-)

>> +
>> +  StructLayout *&operator[](const Type *Key) {
>
> Key should be declared as 'const StructType *'.
>
Okay.

>> +    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'.
>
I reworked a lot of this so that it's arguably more correct now...I  
hope at least.

-bw