[llvm-commits] Updated TargetData patch.

[Chris Lattner]

On Feb 6, 2007, at 10:33 AM, Scott Michel wrote:
> Found a minor bug in the previous patch (bit<->byte conversion),
> fixed 80col violations (hopefully), got rid of tabs.

Some nits:

  #include <vector>
+#include "llvm/ADT/SmallVector.h"

Do you need <vector> anymore?  If not, please remove the #include.

+struct TargetAlignElem {
+  unsigned char       AlignType;      //< Alignment type  
(AlignTypeEnum)
+  unsigned char       ABIAlign;       //< ABI alignment for this  
type/bitw
+  unsigned char       PrefAlign;      //< Preferred alignment for  
this type/bitw
+  short               TypeBitWidth;   //< Type bit width
+
+  /// Default constructor
+  TargetAlignElem();
+  /// Full constructor
+  TargetAlignElem(AlignTypeEnum align_type, unsigned char abi_align,
+                  unsigned char pref_align, short bit_width);
+  /// Copy constructor
+  TargetAlignElem(const TargetAlignElem &src);
+  /// Destructor
+  ~TargetAlignElem() { }
+  /// Assignment operator
+  TargetAlignElem &operator=(const TargetAlignElem &rhs);
+  /// Less-than predicate
+  bool operator<(const TargetAlignElem &rhs) const;
+  /// Equality predicate
+  bool operator==(const TargetAlignElem &rhs) const;
+  /// output stream operator
+  std::ostream &dump(std::ostream &os) const;
+}

This class really wants to be a POD.  Please remove all the ctors and  
dtor, and instead use a single:

+struct TargetAlignElem {
+  unsigned char       AlignType;      //< Alignment type  
(AlignTypeEnum)
+  unsigned char       ABIAlign;       //< ABI alignment for this  
type/bitw
+  unsigned char       PrefAlign;      //< Preferred alignment for  
this type/bitw
+  short               TypeBitWidth;   //< Type bit width

      static TargetAlignElem get(AlignTypeEnum align_type, unsigned  
char abi_align,
                    unsigned char pref_align, short bit_width);

method.  This class not being a pod inhibits some compiler  
optimizations.

operator< and operator==  and dump can remain methods.

+/// Target alignment container
+///
+/// This is the container for most primitive types' alignment, i.e.,  
integer,
+/// floating point, vectors and aggregates.
+class TargetAlign : public SmallVector<TargetAlignElem, 16> {

Never inherit from containers.  Please make the smallvector be an  
instance var and add accessors as appropriate.  See Effective C++ for  
justification.

Is there any significant reason not to just 'inline' the TargetAlign  
class into the TargetData class?  If it's just a SmallVector of  
TargetAlignElem's, why not just declare it as such and put the  
accessors on TargetData?


Index: lib/CodeGen/MachineFunction.cpp
===================================================================
--- lib/CodeGen/MachineFunction.cpp	(.../trunk)	(revision 522)
+++ lib/CodeGen/MachineFunction.cpp	(.../branches/llvm-spu)	(revision  
522)
@@ -13,6 +13,8 @@
  //
  // 
===--------------------------------------------------------------------- 
-===//

+#include "llvm/Type.h"
+#include "llvm/DerivedTypes.h"

DerivedTypes pulls in Type.h, so you can drop the first #include.

@@ -123,7 +125,7 @@
    const TargetData &TD = *TM.getTargetData();
    bool IsPic = TM.getRelocationModel() == Reloc::PIC_;
    unsigned EntrySize = IsPic ? 4 : TD.getPointerSize();
-  unsigned Alignment = IsPic ? TD.getIntABIAlignment()
+  unsigned Alignment = IsPic ? TD.getABITypeAlignment(Type::Int32Ty)
                               : TD.getPointerABIAlignment();


Index: lib/Target/TargetData.cpp
===================================================================
--- lib/Target/TargetData.cpp	(.../trunk)	(revision 522)
+++ lib/Target/TargetData.cpp	(.../branches/llvm-spu)	(revision 522)
@@ -23,6 +23,7 @@
  #include "llvm/Support/GetElementPtrTypeIterator.h"
  #include "llvm/Support/MathExtras.h"
  #include "llvm/ADT/StringExtras.h"
+#include <iostream>
  #include <algorithm>


<iostream> is verboten, please see the coding standards doc.



Overall, the new design is nice.  It is much less a bag of random  
bits! :)  Please resubmit the patch with the changes, thanks!

-Chris



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070206/5d976587/attachment.html>