llvm dev - Jul 2008 - [LLVMdev] Casting between address spaces and address space semantics

Hi all,
> If I read the standard correctly, the properties of these address spaces
can
> be fully captured by defining the relationship between every pair of
address
> spaces (disjoint, identical, subset/superset).
> 
> I think it would make sense to make these relationships backend/platform
> specific, but for clang and the optimization passes to properly work with
> address spaces, these relationships should be known. It seems to me that
the
> proper way to handle this, is to include this information in the TargetData
> class (or whatever platform-specific class is appropriate, I'm not
really into
> this currently).
To have something to actually talk about, I thought I'd create some code.
I've
got a patch attached which consists roughly of two parts: it adds the concept
of address space relations to TargetData, and teaches InstructionCombining to
remove casts based on those relations.

The instcombine is pretty straightforward: If a bitcast changes the address
space of a pointer in a way that is redundant, and all of the users of the
bitcasts can be rewritten to use the original pointer as well (currently this
means loads, stores and geps only), this is done.

The TargetData part is a lot more tricky. I originally planned to simply add a
virtual method to TargetData and create a specific subclass for our compiler,
when I discovered that this is not how TargetData is supposed to work.
Currently, TargetData consists only of a TargetDescription, which is basically
only sizes and alignment info for different types.

I solved this by creating a new class AddrspacesDescriptor, which has a single
virtual method that defines the relationship between address spaces. This
class contains a default implementation and can be subclassed to get a
different implementation.

A reference to a AddrspaceDescriptor is passed to TargetData's constructor,
with a default. This means that all of the current code uses the default
implementation. I'm not so sure what would be the correct way to make the
various tool select other configurations, currently.

Also, I'm quite positive that TargetData is not really the right place to
put
something like this. At least not in the way it is currently written, passing
around references to AddrspacesDescriptor is far from ideal. However, I do not
think we should be trying to define these relations between address spaces in
a text-based format similar to the current TargetDescription. In that way, we
would either loose a lot of flexibility, or end up with a huge
TargetDescription (with one element for each combination of address
spaces...).

So, any comments on the patch and/or hints on how to do things better? Any
thoughts on how selecting a address spaces configuration should work in the
different tools?

Gr.

Matthijs

Index: lib/Transforms/Scalar/InstructionCombining.cpp
==================================================================---
lib/Transforms/Scalar/InstructionCombining.cpp	(revision 53716)
+++ lib/Transforms/Scalar/InstructionCombining.cpp	(working copy)
@@ -400,6 +400,8 @@
     unsigned GetOrEnforceKnownAlignment(Value *V,
                                         unsigned PrefAlign = 0);
 
+    /// Propagate address spaces through bitcasts
+    Instruction *PropagateAddressSpace(BitCastInst &CI);
   };
 }
 
@@ -7841,6 +7843,81 @@
   return 0;
 }
 
+/// Try to propagate the address space from the source of the bitcast into all
+/// of its uses. CI casts between two pointer types with different address
spaces.
+Instruction *InstCombiner::PropagateAddressSpace(BitCastInst &CI) {
+  // Determine the new type. This is the old dest type with the address space
+  // changed. We can't simply use the source type, since the element type
can be
+  // changed in the cast in addition to the address space.
+  const PointerType *DestTy = cast<PointerType>(CI.getDestTy());
+  const PointerType *SrcTy  = cast<PointerType>(CI.getSrcTy());
+  const PointerType *NTy = PointerType::get(DestTy->getElementType(),
SrcTy->getAddressSpace());
+    
+  // Only propagate the address space when the source and dest address spaces
+  // are equivalent, or the source address space is a subset of the target
+  // address space.
+  AddrspacesDescriptor::AddrspaceRelation Rel;
+  Rel = TD->getAddrspaceRelation(SrcTy->getAddressSpace(),
+                                 DestTy->getAddressSpace());
+  if (Rel != AddrspacesDescriptor::Equivalent 
+      && Rel != AddrspacesDescriptor::Subset)
+    return 0;
+
+  // We can propagate the address space if we can modify the type of CI. This
+  // requires that any uses of it should be modifiable. For now, we'll just
+  // propagate if all uses are loads and stores.
+  for (User::use_iterator I = CI.use_begin(), E = CI.use_end(); I != E; ++I) {
+    if (isa<StoreInst>(*I)) {
+      if (I.getOperandNo() != StoreInst::getPointerOperandIndex())
+        return 0; // Can only handle store to this pointer
+    } else if (isa<GetElementPtrInst>(*I)) {
+      if (I.getOperandNo() != GetElementPtrInst::getPointerOperandIndex())
+        return 0;
+    } else if (!isa<LoadInst>(*I)) {
+      // Can't handle other instructions
+      return 0;
+    }
+  }
+  // Insert a new bitcast, if needed
+  Value *NewVal = InsertBitCastBefore(CI.getOperand(0), NTy, CI);
+  
+  for (User::use_iterator I = CI.use_begin(), E = CI.use_end(); I != E; ++I) {
+    Instruction *NewInst;
+    Instruction *OldInst = cast<Instruction>(*I);
+    // Declare this here, so we can reuse the dyn_cast
+    GetElementPtrInst *GEP = NULL;
+    // Create a new instruction
+    if (isa<StoreInst>(*I))
+      NewInst = new StoreInst(I->getOperand(0), NewVal);
+    else if ((GEP = dyn_cast<GetElementPtrInst>(*I))) {
+      // Insert a new GEP
+      std::vector<Value*> Indices(GEP->idx_begin(),
GEP->idx_end());
+      NewInst = GetElementPtrInst::Create(NewVal, Indices.begin(),
Indices.end());
+    } else // Must be LoadInst
+      NewInst = new LoadInst(NewVal);
+    
+    // Replace the old instruction
+    InsertNewInstBefore(NewInst, *OldInst);
+    NewInst->takeName(OldInst);
+    
+    // Put this in a Value*, since InsertBitCastBefore could return a Value*
+    // below.
+    Value *ReplaceWith = NewInst;
+    if (GEP) {
+      // Now insert a bitcast as well. We couldn't do this before, because
the
+      // above two lines don't hold for the GEPs
+      const Type* GEPTy =
PointerType::get(GEP->getType()->getElementType(),
DestTy->getAddressSpace());
+      ReplaceWith = InsertBitCastBefore(NewInst, GEPTy, *GEP);
+    }
+    ReplaceInstUsesWith(*OldInst, ReplaceWith);
+    EraseInstFromFunction(*OldInst);
+
+  }
+  
+  // Return the old instruction, so it can be removed.
+  return &CI;
+}
+
 Instruction *InstCombiner::visitBitCast(BitCastInst &CI) {
   // If the operands are integer typed then apply the integer transforms,
   // otherwise just apply the common ones.
@@ -7873,7 +7950,9 @@
     // If the address spaces don't match, don't eliminate the bitcast,
which is
     // required for changing types.
     if (SrcPTy->getAddressSpace() != DstPTy->getAddressSpace())
-      return 0;
+      // If we're casting to address space zero, see if we can't just
propagate
+      // the address space into the uses of CI.
+      return PropagateAddressSpace(CI);
     
     // If we are casting a malloc or alloca to a pointer to a type of the same
     // size, rewrite the allocation instruction to allocate the
"right" type.
Index: lib/Target/TargetData.cpp
==================================================================---
lib/Target/TargetData.cpp	(revision 53716)
+++ lib/Target/TargetData.cpp	(working copy)
@@ -231,8 +231,8 @@
   }
 }
 
-TargetData::TargetData(const Module *M) 
-  : ImmutablePass((intptr_t)&ID) {
+TargetData::TargetData(const Module *M, const AddrspacesDescriptor
&Addrspaces)
+  : ImmutablePass((intptr_t)&ID), Addrspaces(Addrspaces) {
   init(M->getDataLayout());
 }
 
@@ -604,3 +604,5 @@
 unsigned TargetData::getPreferredAlignmentLog(const GlobalVariable *GV) const {
   return Log2_32(getPreferredAlignment(GV));
 }
+
+const AddrspacesDescriptor TargetData::DefaultAddrspaces;
Index: include/llvm/Target/TargetData.h
==================================================================---
include/llvm/Target/TargetData.h	(revision 53716)
+++ include/llvm/Target/TargetData.h	(working copy)
@@ -63,12 +63,41 @@
   std::ostream &dump(std::ostream &os) const;
 };
 
+/// This class is used to describe relations between different address spaces.
+/// The default implementation defines all different address spaces as
+/// equivalent.
+///
+/// Create a subclass and pass it to TargetData's constructors to define
+/// different behaviour.
+class AddrspacesDescriptor {
+public:
+  typedef enum {
+    Equivalent,
+    Disjoint,
+    Subset,
+    Superset
+  } AddrspaceRelation;
+
+  /// getAddrspaceRelation - Returns the relation between the given address
+  /// spaces. Should be read left to right, for example Subset means that
+  /// Addrspace1 is a subset of Addrspace2.
+  virtual AddrspaceRelation getAddrspaceRelation(
+    int Addrspace1,
+    int Addrspace2) const {
+      return Equivalent;
+  }
+
+  virtual ~AddrspacesDescriptor() {}
+};
+
 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 alignment
+  const AddrspacesDescriptor &Addrspaces; ///< Address spaces descriptor
+  static const AddrspacesDescriptor DefaultAddrspaces; ///< Default address
spaces descriptor
 
   //! Where the primitive type alignment data is stored.
   /*!
@@ -109,20 +138,23 @@
   ///
   /// @note This has to exist, because this is a pass, but it should never be
   /// used.
-  TargetData() : ImmutablePass(intptr_t(&ID)) {
+  TargetData() : ImmutablePass(intptr_t(&ID)),
Addrspaces(DefaultAddrspaces) {
     assert(0 && "ERROR: Bad TargetData ctor used.  "
            "Tool did not specify a TargetData to use?");
     abort();
   }
     
   /// Constructs a TargetData from a specification string. See init().
-  explicit TargetData(const std::string &TargetDescription) 
-    : ImmutablePass(intptr_t(&ID)) {
+  explicit TargetData(const std::string &TargetDescription,
+                      const AddrspacesDescriptor &Addrspaces
+                      = DefaultAddrspaces)
+    : ImmutablePass(intptr_t(&ID)), Addrspaces(Addrspaces) {
     init(TargetDescription);
   }
 
   /// Initialize target data from properties stored in the module.
-  explicit TargetData(const Module *M);
+  explicit TargetData(const Module *M, const AddrspacesDescriptor
&Addrspaces
+                                       = DefaultAddrspaces);
 
   TargetData(const TargetData &TD) : 
     ImmutablePass(intptr_t(&ID)),
@@ -130,6 +162,7 @@
     PointerMemSize(TD.PointerMemSize),
     PointerABIAlign(TD.PointerABIAlign),
     PointerPrefAlign(TD.PointerPrefAlign),
+    Addrspaces(TD.Addrspaces),
     Alignments(TD.Alignments)
   { }
 
@@ -240,6 +273,15 @@
   /// requested alignment (if the global has one).
   unsigned getPreferredAlignmentLog(const GlobalVariable *GV) const;
 
+  /// getAddrspaceRelation - Returns the relation between the given address
+  /// spaces. Should be read left to right, for example Subset means that
+  /// Addrspace1 is a subset of Addrspace2.
+  AddrspacesDescriptor::AddrspaceRelation getAddrspaceRelation(int Addrspace1,
+                                                               int Addrspace2)
{
+                            
+    return Addrspaces.getAddrspaceRelation(Addrspace1, Addrspace2);
+  }
+
   static char ID; // Pass identification, replacement for typeid
 };
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080721/e5fb5120/attachment.sig>

Hi Matthijs,

Thanks for giving some code so we can discuss this in more concrete  
detail.  In terms of the information we need, I think you have it  
right.  We just need a description of how the different address spaces  
relate and I don't see much of an issue with how you implemented to  
InstructionCombining.

As you also mentioned, I don't like that we pass a reference to  
TargetData with the AddrspacesDescriptor and that we have a static  
default implementation store in the class.  It is unclear to me if  
this is the right class for it as not as it describes the structure  
and alignment while address spaces seems distinct from that.  I feel  
that this information should be part of the TargetMachine as the  
address spaces description is part of a target machine but I may be  
wrong here so if someone has a different opinion here, please chime in.

In terms of text based or not, I don't have a strong opinion.  I don't  
see a problem with a text based format as these relationships are  
pretty simple since there can't be weird overlaps.  With only the  
choice of disjoint, subset, and equivalent, the description shouldn't  
be too long and the tool can compute the transitive closure of the  
description and generate a reasonable efficient table for lookup.  I  
don't see a problem with defining one's own class to store this  
information either.

As an aside, I think that the default relationship for two different  
address spaces should be disjoint.  We should not encourage people to  
define equivalent address spaces as in general, I don't see why  
someone would want to do that except in rare circumstances.

   -- Mon Ping


On Jul 21, 2008, at 8:05 AM, Matthijs Kooijman wrote:
> Hi all,
>
>> If I read the standard correctly, the properties of these address  
>> spaces can
>> be fully captured by defining the relationship between every pair  
>> of address
>> spaces (disjoint, identical, subset/superset).
>>
>> I think it would make sense to make these relationships backend/ 
>> platform
>> specific, but for clang and the optimization passes to properly  
>> work with
>> address spaces, these relationships should be known. It seems to me  
>> that the
>> proper way to handle this, is to include this information in the  
>> TargetData
>> class (or whatever platform-specific class is appropriate, I'm not
>> really into
>> this currently).
>
> To have something to actually talk about, I thought I'd create some  
> code. I've
> got a patch attached which consists roughly of two parts: it adds  
> the concept
> of address space relations to TargetData, and teaches  
> InstructionCombining to
> remove casts based on those relations.
>
> The instcombine is pretty straightforward: If a bitcast changes the  
> address
> space of a pointer in a way that is redundant, and all of the users  
> of the
> bitcasts can be rewritten to use the original pointer as well  
> (currently this
> means loads, stores and geps only), this is done.
>
> The TargetData part is a lot more tricky. I originally planned to  
> simply add a
> virtual method to TargetData and create a specific subclass for our  
> compiler,
> when I discovered that this is not how TargetData is supposed to work.
> Currently, TargetData consists only of a TargetDescription, which is  
> basically
> only sizes and alignment info for different types.
>
> I solved this by creating a new class AddrspacesDescriptor, which  
> has a single
> virtual method that defines the relationship between address spaces.  
> This
> class contains a default implementation and can be subclassed to get a
> different implementation.
>
> A reference to a AddrspaceDescriptor is passed to TargetData's  
> constructor,
> with a default. This means that all of the current code uses the  
> default
> implementation. I'm not so sure what would be the correct way to  
> make the
> various tool select other configurations, currently.
>
> Also, I'm quite positive that TargetData is not really the right  
> place to put
> something like this. At least not in the way it is currently  
> written, passing
> around references to AddrspacesDescriptor is far from ideal.  
> However, I do not
> think we should be trying to define these relations between address  
> spaces in
> a text-based format similar to the current TargetDescription. In  
> that way, we
> would either loose a lot of flexibility, or end up with a huge
> TargetDescription (with one element for each combination of address
> spaces...).
>
> So, any comments on the patch and/or hints on how to do things  
> better? Any
> thoughts on how selecting a address spaces configuration should work  
> in the
> different tools?
>
> Gr.
>
> Matthijs
>
> Index: lib/Transforms/Scalar/InstructionCombining.cpp
> ==================================================================> ---
lib/Transforms/Scalar/InstructionCombining.cpp	(revision 53716)
> +++ lib/Transforms/Scalar/InstructionCombining.cpp	(working copy)
> @@ -400,6 +400,8 @@
>     unsigned GetOrEnforceKnownAlignment(Value *V,
>                                         unsigned PrefAlign = 0);
>
> +    /// Propagate address spaces through bitcasts
> +    Instruction *PropagateAddressSpace(BitCastInst &CI);
>   };
> }
>
> @@ -7841,6 +7843,81 @@
>   return 0;
> }
>
> +/// Try to propagate the address space from the source of the  
> bitcast into all
> +/// of its uses. CI casts between two pointer types with different  
> address spaces.
> +Instruction *InstCombiner::PropagateAddressSpace(BitCastInst &CI) {
> +  // Determine the new type. This is the old dest type with the  
> address space
> +  // changed. We can't simply use the source type, since the  
> element type can be
> +  // changed in the cast in addition to the address space.
> +  const PointerType *DestTy = cast<PointerType>(CI.getDestTy());
> +  const PointerType *SrcTy  = cast<PointerType>(CI.getSrcTy());
> +  const PointerType *NTy = PointerType::get(DestTy- 
> >getElementType(), SrcTy->getAddressSpace());
> +
> +  // Only propagate the address space when the source and dest  
> address spaces
> +  // are equivalent, or the source address space is a subset of the  
> target
> +  // address space.
> +  AddrspacesDescriptor::AddrspaceRelation Rel;
> +  Rel = TD->getAddrspaceRelation(SrcTy->getAddressSpace(),
> +                                 DestTy->getAddressSpace());
> +  if (Rel != AddrspacesDescriptor::Equivalent
> +      && Rel != AddrspacesDescriptor::Subset)
> +    return 0;
> +
> +  // We can propagate the address space if we can modify the type  
> of CI. This
> +  // requires that any uses of it should be modifiable. For now,  
> we'll just
> +  // propagate if all uses are loads and stores.
> +  for (User::use_iterator I = CI.use_begin(), E = CI.use_end(); I ! 
> = E; ++I) {
> +    if (isa<StoreInst>(*I)) {
> +      if (I.getOperandNo() != StoreInst::getPointerOperandIndex())
> +        return 0; // Can only handle store to this pointer
> +    } else if (isa<GetElementPtrInst>(*I)) {
> +      if (I.getOperandNo() !=  
> GetElementPtrInst::getPointerOperandIndex())
> +        return 0;
> +    } else if (!isa<LoadInst>(*I)) {
> +      // Can't handle other instructions
> +      return 0;
> +    }
> +  }
> +  // Insert a new bitcast, if needed
> +  Value *NewVal = InsertBitCastBefore(CI.getOperand(0), NTy, CI);
> +
> +  for (User::use_iterator I = CI.use_begin(), E = CI.use_end(); I ! 
> = E; ++I) {
> +    Instruction *NewInst;
> +    Instruction *OldInst = cast<Instruction>(*I);
> +    // Declare this here, so we can reuse the dyn_cast
> +    GetElementPtrInst *GEP = NULL;
> +    // Create a new instruction
> +    if (isa<StoreInst>(*I))
> +      NewInst = new StoreInst(I->getOperand(0), NewVal);
> +    else if ((GEP = dyn_cast<GetElementPtrInst>(*I))) {
> +      // Insert a new GEP
> +      std::vector<Value*> Indices(GEP->idx_begin(),
GEP->idx_end());
> +      NewInst = GetElementPtrInst::Create(NewVal, Indices.begin(),  
> Indices.end());
> +    } else // Must be LoadInst
> +      NewInst = new LoadInst(NewVal);
> +
> +    // Replace the old instruction
> +    InsertNewInstBefore(NewInst, *OldInst);
> +    NewInst->takeName(OldInst);
> +
> +    // Put this in a Value*, since InsertBitCastBefore could return  
> a Value*
> +    // below.
> +    Value *ReplaceWith = NewInst;
> +    if (GEP) {
> +      // Now insert a bitcast as well. We couldn't do this before,  
> because the
> +      // above two lines don't hold for the GEPs
> +      const Type* GEPTy = PointerType::get(GEP->getType()- 
> >getElementType(), DestTy->getAddressSpace());
> +      ReplaceWith = InsertBitCastBefore(NewInst, GEPTy, *GEP);
> +    }
> +    ReplaceInstUsesWith(*OldInst, ReplaceWith);
> +    EraseInstFromFunction(*OldInst);
> +
> +  }
> +
> +  // Return the old instruction, so it can be removed.
> +  return &CI;
> +}
> +
> Instruction *InstCombiner::visitBitCast(BitCastInst &CI) {
>   // If the operands are integer typed then apply the integer  
> transforms,
>   // otherwise just apply the common ones.
> @@ -7873,7 +7950,9 @@
>     // If the address spaces don't match, don't eliminate the  
> bitcast, which is
>     // required for changing types.
>     if (SrcPTy->getAddressSpace() != DstPTy->getAddressSpace())
> -      return 0;
> +      // If we're casting to address space zero, see if we can't  
> just propagate
> +      // the address space into the uses of CI.
> +      return PropagateAddressSpace(CI);
>
>     // If we are casting a malloc or alloca to a pointer to a type  
> of the same
>     // size, rewrite the allocation instruction to allocate the  
> "right" type.
> Index: lib/Target/TargetData.cpp
> ==================================================================> ---
lib/Target/TargetData.cpp	(revision 53716)
> +++ lib/Target/TargetData.cpp	(working copy)
> @@ -231,8 +231,8 @@
>   }
> }
>
> -TargetData::TargetData(const Module *M)
> -  : ImmutablePass((intptr_t)&ID) {
> +TargetData::TargetData(const Module *M, const AddrspacesDescriptor  
> &Addrspaces)
> +  : ImmutablePass((intptr_t)&ID), Addrspaces(Addrspaces) {
>   init(M->getDataLayout());
> }
>
> @@ -604,3 +604,5 @@
> unsigned TargetData::getPreferredAlignmentLog(const GlobalVariable  
> *GV) const {
>   return Log2_32(getPreferredAlignment(GV));
> }
> +
> +const AddrspacesDescriptor TargetData::DefaultAddrspaces;
> Index: include/llvm/Target/TargetData.h
> ==================================================================> ---
include/llvm/Target/TargetData.h	(revision 53716)
> +++ include/llvm/Target/TargetData.h	(working copy)
> @@ -63,12 +63,41 @@
>   std::ostream &dump(std::ostream &os) const;
> };
>
> +/// This class is used to describe relations between different  
> address spaces.
> +/// The default implementation defines all different address spaces  
> as
> +/// equivalent.
> +///
> +/// Create a subclass and pass it to TargetData's constructors to  
> define
> +/// different behaviour.
> +class AddrspacesDescriptor {
> +public:
> +  typedef enum {
> +    Equivalent,
> +    Disjoint,
> +    Subset,
> +    Superset
> +  } AddrspaceRelation;
> +
> +  /// getAddrspaceRelation - Returns the relation between the given  
> address
> +  /// spaces. Should be read left to right, for example Subset  
> means that
> +  /// Addrspace1 is a subset of Addrspace2.
> +  virtual AddrspaceRelation getAddrspaceRelation(
> +    int Addrspace1,
> +    int Addrspace2) const {
> +      return Equivalent;
> +  }
> +
> +  virtual ~AddrspacesDescriptor() {}
> +};
> +
> 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  
> alignment
> +  const AddrspacesDescriptor &Addrspaces; ///< Address spaces  
> descriptor
> +  static const AddrspacesDescriptor DefaultAddrspaces; ///< Default  
> address spaces descriptor
>
>   //! Where the primitive type alignment data is stored.
>   /*!
> @@ -109,20 +138,23 @@
>   ///
>   /// @note This has to exist, because this is a pass, but it should  
> never be
>   /// used.
> -  TargetData() : ImmutablePass(intptr_t(&ID)) {
> +  TargetData() : ImmutablePass(intptr_t(&ID)),  
> Addrspaces(DefaultAddrspaces) {
>     assert(0 && "ERROR: Bad TargetData ctor used.  "
>            "Tool did not specify a TargetData to use?");
>     abort();
>   }
>
>   /// Constructs a TargetData from a specification string. See init().
> -  explicit TargetData(const std::string &TargetDescription)
> -    : ImmutablePass(intptr_t(&ID)) {
> +  explicit TargetData(const std::string &TargetDescription,
> +                      const AddrspacesDescriptor &Addrspaces
> +                      = DefaultAddrspaces)
> +    : ImmutablePass(intptr_t(&ID)), Addrspaces(Addrspaces) {
>     init(TargetDescription);
>   }
>
>   /// Initialize target data from properties stored in the module.
> -  explicit TargetData(const Module *M);
> +  explicit TargetData(const Module *M, const AddrspacesDescriptor  
> &Addrspaces
> +                                       = DefaultAddrspaces);
>
>   TargetData(const TargetData &TD) :
>     ImmutablePass(intptr_t(&ID)),
> @@ -130,6 +162,7 @@
>     PointerMemSize(TD.PointerMemSize),
>     PointerABIAlign(TD.PointerABIAlign),
>     PointerPrefAlign(TD.PointerPrefAlign),
> +    Addrspaces(TD.Addrspaces),
>     Alignments(TD.Alignments)
>   { }
>
> @@ -240,6 +273,15 @@
>   /// requested alignment (if the global has one).
>   unsigned getPreferredAlignmentLog(const GlobalVariable *GV) const;
>
> +  /// getAddrspaceRelation - Returns the relation between the given  
> address
> +  /// spaces. Should be read left to right, for example Subset  
> means that
> +  /// Addrspace1 is a subset of Addrspace2.
> +  AddrspacesDescriptor::AddrspaceRelation getAddrspaceRelation(int  
> Addrspace1,
> +                                                               int  
> Addrspace2) {
> +
> +    return Addrspaces.getAddrspaceRelation(Addrspace1, Addrspace2);
> +  }
> +
>   static char ID; // Pass identification, replacement for typeid
> };
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Hi Mon Ping,
> As you also mentioned, I don't like that we pass a reference to  
> TargetData with the AddrspacesDescriptor and that we have a static  
> default implementation store in the class.  It is unclear to me if  
> this is the right class for it as not as it describes the structure  
> and alignment while address spaces seems distinct from that.  I feel  
> that this information should be part of the TargetMachine as the  
> address spaces description is part of a target machine but I may be  
> wrong here so if someone has a different opinion here, please chime in.
From the looks of it, TargetMachine was only used in the codegenerator, and
the other transformation passes can't access it. Or is that a wrong
observation on my side?

Anyway, since this information is needed pretty much over the entire field
(ie, even clang should have it to know whether an implicit should be generated
or cause an error), TargetMachine might be way too low level. TargetData seems
okay, but might need a bit of a redesign.

Perhaps some way of coding these relations into the LLVM IR would be needed as
well? Or perhaps we could load the right set of relations based on the target
triple?
> In terms of text based or not, I don't have a strong opinion.  I
don't
> see a problem with a text based format as these relationships are  
> pretty simple since there can't be weird overlaps.  With only the  
> choice of disjoint, subset, and equivalent, the description shouldn't  
> be too long and the tool can compute the transitive closure of the  
> description and generate a reasonable efficient table for lookup.
Hmm, you have a good point there. Transitivity should greatly limit the number
of possible options.
> I  don't see a problem with defining one's own class to store this
> information either.
Let's see about this later on then :-)
> As an aside, I think that the default relationship for two different
> address spaces should be disjoint.  We should not encourage people to
> define equivalent address spaces as in general, I don't see why 
someone
> would want to do that except in rare circumstances.
Yeah, that made more sense to me as well. However, I set it to Equivalent for
now, to be compatible with what clang generates. If we make clang listen to
this code as well, changing the default to Disjoint makes a lot of sense.

So, can anyone shed some light on the TargetData/Machine/Whatever issue?

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080721/954ece73/attachment.sig>