Tue Dec 11 18:09:17 PST 2007

On Dec 11, 2007, at 4:12 PM, Chris Lattner wrote:

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

Ok.

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

This is true.

> Do you think this  is the best way to go?

As opposed to no default value, forcing clients to propagate this  
information?

> Do many clients of PointerType::get need to  be aware of addr spaces?

Many do. Unfortunately it seems common to simply pass a value's type  
around and synthesize a pointer when needed. The addition of address  
spaces means that this approach is no longer equivalent to passing  
the pointer type, and I expect there will be fixes needed to once a  
back end depends on this feature.

>> ===================================================================== 
>> =
>> ========
>> --- 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?

Avoiding the duplicate production is good, but I'm not sure about the  
comma. The addrspace() qualifier can not be positionally permuted  
with other attributes, the comma gives the impression that it can be.

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

I will have to check.

>
>
>> ===================================================================== 
>> =
>> ========
>> --- 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.

Ok.

>
>> 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!

This goes back to the point about the comma. The addrspace()  
qualifier can not be positionally permuted with other attributes.

--
Christopher Lamb

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