Tue Dec 11 16:12:15 PST 2007

On Dec 11, 2007, at 12:59 AM, Christopher Lamb wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=44858&view=rev
> Log:
> Implement address space attribute for LLVM pointer types. Address  
> spaces are
> regions of memory that have a target specific relationship, as  
> described in the
> Embedded C Technical Report.

Woohoo!    Minor stuff:

> +++ llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h Tue Dec 11  
> 02:59:05 2007
> @@ -194,9 +194,13 @@
>      FUNC_CODE_INST_FREE        = 18, // FREE:       [opty, op]
>      FUNC_CODE_INST_ALLOCA      = 19, // ALLOCA:     [instty, op,  
> align]
>      FUNC_CODE_INST_LOAD        = 20, // LOAD:       [opty, op,  
> align, vol]
> -    FUNC_CODE_INST_STORE       = 21, // STORE:       
> [ptrty,val,ptr, align, vol]
> +    FUNC_CODE_INST_STORE       = 21, // STORE:       
> [valty,val,ptr, align, vol]
>      FUNC_CODE_INST_CALL        = 22, // CALL:       [attr, fnty,  
> fnid, args...]
> -    FUNC_CODE_INST_VAARG       = 23  // VAARG:      [valistty,  
> valist, instty]
> +    FUNC_CODE_INST_VAARG       = 23, // VAARG:      [valistty,  
> valist, instty]
> +    // This store code encodes the pointer type, rather than the  
> value type
> +    // this is so information only available in the pointer type  
> (e.g. address
> +    // spaces) is retained.

Please add a "FIXME: Remove this in LLVM 3.0" to FUNC_CODE_INST_STORE.

> +++ llvm/trunk/include/llvm/DerivedTypes.h Tue Dec 11 02:59:05 2007
> @@ -363,12 +363,17 @@
>  ///
>  class PointerType : public SequentialType {
>    friend class TypeMap<PointerValType, PointerType>;
> +  unsigned AddressSpace;
> +
>    PointerType(const PointerType &);                   // Do not  
> implement
>    const PointerType &operator=(const PointerType &);  // Do not  
> implement
> -  explicit PointerType(const Type *ElType);
> +  explicit PointerType(const Type *ElType, unsigned AddrSpace);
>  public:
>    /// PointerType::get - This is the only way to construct a new  
> pointer type.
> -  static PointerType *get(const Type *ElementType);
> +  static PointerType *get(const Type *ElementType, unsigned  
> AddressSpace = 0);

Making the address space default to zero is convenient, but  
dangerous.  This means that xforms that play with pointers need to be  
very careful to propagate this info in some cases.  Do you think this  
is the best way to go?  Do many clients of PointerType::get need to  
be aware of addr spaces?

> ====================================================================== 
> ========
> --- llvm/trunk/lib/AsmParser/llvmAsmParser.y (original)
> +++ llvm/trunk/lib/AsmParser/llvmAsmParser.y Tue Dec 11 02:59:05 2007
> @@ -1320,6 +1323,13 @@
>      delete $1;
>      CHECK_FOR_ERROR
>    }
> +  | Types ADDRSPACE '(' EUINT64VAL ')' '*' {             //  
> Pointer type?
> +    if (*$1 == Type::LabelTy)
> +      GEN_ERROR("Cannot form a pointer to a basic block");
> +    $$ = new PATypeHolder(HandleUpRefs(PointerType::get(*$1, $4)));
> +    delete $1;
> +    CHECK_FOR_ERROR
> +  }

It would probably be better to factor this as an new production:

OptAddrSpace ::= ADDRSPACE '(' EUINT64VAL ')' { $$=$3; }
OptAddrSpace ::= /*empty*/ { $$ = 0; }

Which thing allows you to change the current pointer production to:

   | Types OptAddrSpace '*' {                             // Pointer  
type?
     if (*$1 == Type::LabelTy)
       GEN_ERROR("Cannot form a pointer to a basic block");
     $$ = new PATypeHolder(HandleUpRefs(PointerType::get(*$1, $2)));
     delete $1;
     CHECK_FOR_ERROR

This becomes useful later when:

> @@ -2073,6 +2083,17 @@
>    } GlobalVarAttributes {
>      CurGV = 0;
>    }
> +  | OptGlobalAssign GVVisibilityStyle ThreadLocal GlobalType ConstVal
> +    ADDRSPACE '(' EUINT64VAL ')' {
> +    /* "Externally Visible" Linkage with address space qualifier */
> +    if ($5 == 0)
> +      GEN_ERROR("Global value initializer is not a constant");
> +    CurGV = ParseGlobalVariable($1, GlobalValue::ExternalLinkage,
> +                                $2, $4, $5->getType(), $5, $3, $8);
> +    CHECK_FOR_ERROR
> +  } GlobalVarAttributes {
> +    CurGV = 0;
> +  }

It would be nice to use the above stuff to avoid duplicating this  
production.  Maybe it would need to be:

OptCommaAddrSpace ::= ',' ADDRSPACE '(' EUINT64VAL ')' { $$=$4; }
OptCommaAddrSpace ::= /*empty*/ { $$ = 0; }

And then just add OptCommaAddrSpace to the existing production.  What  
do you think?

Also, should it be possible to place a function in an address space?   
Does the embedded C spec allow this?

> ====================================================================== 
> ========
> --- llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp (original)
> +++ llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp Tue Dec 11  
> 02:59:05 2007
> @@ -197,10 +197,14 @@
>        TypeVals.push_back(cast<IntegerType>(T)->getBitWidth());
>        break;
>      case Type::PointerTyID:
> +      const PointerType *PTy = cast<PointerType>(T);
> +      // POINTER: [pointee type] or [pointee type, address space]
>        Code = bitc::TYPE_CODE_POINTER;
> +      TypeVals.push_back(VE.getTypeID(PTy->getElementType()));
> +      if (unsigned AddressSpace = PTy->getAddressSpace())
> +        TypeVals.push_back(AddressSpace);
> +      else
> +        AbbrevToUse = PtrAbbrev;
>        break;

This can be simplified.  In this code, I'd just unconditionally emit it:

>      case Type::PointerTyID:
> +      const PointerType *PTy = cast<PointerType>(T);
> +      // POINTER: [pointee type] or [pointee type, address space]
>        Code = bitc::TYPE_CODE_POINTER;
> +      TypeVals.push_back(VE.getTypeID(PTy->getElementType()));
>        TypeVals.push_back(PTy->getAddressSpace());
>        AbbrevToUse = PtrAbbrev;
>        break;

And change the abbreviation to match, which would apply only if the  
addrspace is zero (and thus not encode it at all):

   // Abbrev for TYPE_CODE_POINTER.
   BitCodeAbbrev *Abbv = new BitCodeAbbrev();
   Abbv->Add(BitCodeAbbrevOp(bitc::TYPE_CODE_POINTER));
   Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed,
                             Log2_32_Ceil(VE.getTypes().size()+1)));
   Abbv->Add(BitCodeAbbrevOp(0));  // Addr space = 0.
   unsigned PtrAbbrev = Stream.EmitAbbrev(Abbv);

The presence of the abbreviation means that any pointers with  
addrspace 0 will not need per-instance space to represent this.

> Modified: llvm/trunk/lib/VMCore/AsmWriter.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/ 
> AsmWriter.cpp?rev=44858&r1=44857&r2=44858&view=diff
>
> ====================================================================== 
> ========
> --- llvm/trunk/lib/VMCore/AsmWriter.cpp (original)
> +++ llvm/trunk/lib/VMCore/AsmWriter.cpp Tue Dec 11 02:59:05 2007
> @@ -951,6 +955,9 @@
>      writeOperand(GV->getInitializer(), false);
>    }
>
> +  if (unsigned AddressSpace = GV->getType()->getAddressSpace())
> +    Out << " addrspace(" << AddressSpace << ") ";

Comma please.

Overall, very very nice Christopher, thanks for tackling this!

-Chris