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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
