[llvm-commits] CVS: llvm/include/llvm/Target/TargetData.h

[Chris Lattner]

On Feb 13, 2007, at 9:52 PM, Chris Lattner wrote:
> Generalize TargetData strings, to support more interesting forms of  
> data.
> Patch by Scott Michel.

Scott,

> Index: llvm/include/llvm/Target/TargetData.h
> diff -u llvm/include/llvm/Target/TargetData.h:1.53 llvm/include/ 
> llvm/Target/TargetData.h:1.54
> --- llvm/include/llvm/Target/TargetData.h:1.53	Sat Feb 10 14:37:40  
> 2007
> +++ llvm/include/llvm/Target/TargetData.h	Tue Feb 13 23:52:16 2007
> @@ -22,6 +22,8 @@
>
>  #include "llvm/Pass.h"
>  #include "llvm/Support/DataTypes.h"
> +#include "llvm/ADT/SmallVector.h"
> +#include <string>
>
>  namespace llvm {
>
> @@ -31,45 +33,96 @@
>  class StructLayout;
>  class GlobalVariable;
>
> -class TargetData : public ImmutablePass {
> -  bool          LittleEndian;          // Defaults to false
> +/// Enum used to categorize the alignment types stored by  
> TargetAlignElem
> +enum AlignTypeEnum {
> +  INTEGER_ALIGN = 'i',               ///< Integer type alignment
> +  PACKED_ALIGN = 'v',                ///< Vector type alignment
> +  FLOAT_ALIGN = 'f',                 ///< Floating point type  
> alignment
> +  AGGREGATE_ALIGN = 'a'              ///< Aggregate alignment
> +};
> +/// Target alignment element.
> +///
> +/// Stores the alignment data associated with a given alignment  
> type (pointer,
> +/// integer, packed/vector, float) and type bit width.
> +///
> +/// @note The unusual order of elements in the structure attempts  
> to reduce
> +/// padding and make the structure slightly more cache friendly.
> +struct TargetAlignElem {
> +  unsigned char       AlignType;      //< Alignment type  
> (AlignTypeEnum)

This should be:

   AlignTypeEnum AlignType : 8;

> +  unsigned char       ABIAlign;       //< ABI alignment for this  
> type/bitw
> +  unsigned char       PrefAlign;      //< Pref. alignment for this  
> type/bitw
> +  short               TypeBitWidth;   //< Type bit width
> +
> +  /// Initializer
> +  static TargetAlignElem get(AlignTypeEnum align_type, unsigned  
> char abi_align,
> +                             unsigned char pref_align, short  
> bit_width);
> +  /// 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;
> +};
>
> -  // ABI alignments
> -  unsigned char BoolABIAlignment;       // Defaults to 1 byte
> -  unsigned char ByteABIAlignment;       // Defaults to 1 byte
> -  unsigned char ShortABIAlignment;      // Defaults to 2 bytes
> -  unsigned char IntABIAlignment;        // Defaults to 4 bytes
> -  unsigned char LongABIAlignment;       // Defaults to 8 bytes
> -  unsigned char FloatABIAlignment;      // Defaults to 4 bytes
> -  unsigned char DoubleABIAlignment;     // Defaults to 8 bytes
> -  unsigned char PointerMemSize;        // Defaults to 8 bytes
> -  unsigned char PointerABIAlignment;    // Defaults to 8 bytes
> -
> -  // Preferred stack/global type alignments
> -  unsigned char BoolPrefAlignment;    // Defaults to BoolABIAlignment
> -  unsigned char BytePrefAlignment;    // Defaults to ByteABIAlignment
> -  unsigned char ShortPrefAlignment;   // Defaults to  
> ShortABIAlignment
> -  unsigned char IntPrefAlignment;     // Defaults to IntABIAlignment
> -  unsigned char LongPrefAlignment;    // Defaults to LongABIAlignment
> -  unsigned char FloatPrefAlignment;   // Defaults to  
> FloatABIAlignment
> -  unsigned char DoublePrefAlignment;  // Defaults to  
> DoubleABIAlignment
> -  unsigned char PointerPrefAlignment; // Defaults to  
> PointerABIAlignment
> -  unsigned char AggMinPrefAlignment;  // Defaults to 0 bytes
> +//! TargetAlignElem output stream inserter
> +/*!
> +  @sa TargetAlignElem::dump()
> + */
> +std::ostream &operator<<(std::ostream &os, const TargetAlignElem  
> &elem);
> +
> +class TargetData : public ImmutablePass {
> +private:
> +  bool          LittleEndian;          ///< Defaults to false
> +  unsigned char PointerMemSize;        ///< Pointer size in bytes
> +  unsigned char PointerABIAlign;       ///< Pointer ABI alignment
> +  unsigned char PointerPrefAlign;      ///< Pointer preferred  
> global alignment

'Pointer preferred global alignment' ?

global what?

> +
> +  //! Where the primitive type alignment data is stored.
> +  /*!
> +   @sa init().
> +   @note Could support multiple size pointer alignments, e.g., 32- 
> bit pointers
> +   vs. 64-bit pointers by extending TargetAlignment, but for now,  
> we don't.
> +   */
> +  SmallVector<TargetAlignElem, 16> Alignments;
> +  //! Alignment iterator shorthand
> +  typedef SmallVector<TargetAlignElem, 16>::iterator align_iterator;

I don't think the elements in 'Alignments' should ever be mutable  
through iterators.  I'd suggest using:
   typedef SmallVector<TargetAlignElem, 16>::const_iterator  
align_iterator;



> +  //! Invalid alignment.
> +  /*!
> +    This member is a signal that a requested alignment type and  
> bit width were
> +    not found in the SmallVector.
> +   */
> +  static const TargetAlignElem InvalidAlignmentElem;
> +
> +  //! Set/initialize target alignments
> +  void setAlignment(AlignTypeEnum align_type, unsigned char  
> abi_align,
> +                    unsigned char pref_align, short bit_width);
> +  //! Get TargetAlignElem from alignment type and bit width
> +  const TargetAlignElem &getAlignment(AlignTypeEnum, short) const;

Please give these arguments names to indicate what they are.

> +  //! Internal helper method that returns requested alignment for  
> type.
> +  unsigned char getAlignment(const Type *Ty, bool abi_or_pref) const;

abi_or_pref doesn't indicate what 'true' means.  Should this be an  
enum with two values?

> -  /// Parse a target data layout string and initialize TargetData  
> members.
> -  ///
> -  /// Parse a target data layout string, initializing the various  
> TargetData
> -  /// members along the way. A TargetData specification string  
> looks like
> -  /// "E-p:64:64-d:64-f:32-l:64-i:32-s:16-b:8-B:8" and specifies the
> -  /// target's endianess, the ABI alignments of various data types  
> and
> -  /// the size of pointers.

The format of this string should be described somewhere.  Perhaps in  
LangRef.html?

-Chris