[cfe-commits] [PATCH] AltiVec vector support - stage 1

Douglas Gregor dgregor at apple.com
Tue Jan 26 13:07:24 PST 2010


Hi John,

I missed this patch in the post-holiday flood; feel free to kick me if more than a few days goes by and I haven't commented on a patch..

Index: include/clang/AST/Type.h

===================================================================
--- include/clang/AST/Type.h	(revision 94011)
+++ include/clang/AST/Type.h	(working copy)
@@ -1602,14 +1611,28 @@

   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
+  bool isAltiVec() const { return AltiVec; }
+  void setAltiVec(bool isAltiVec) { AltiVec = isAltiVec; }
+  
+  bool isPixel() const { return Pixel; }
+  void setPixel(bool isPixel) { Pixel = isPixel; }

Type classes shouldn't have setters in them, because Types are immutable. The AltiVec and Pixel bits should only be passed in via the constructor arguments.

+  bool isImplicitCastable(const VectorType *ToType) const {
+    return((ToType->ElementType == ElementType) &&
+      (ToType->NumElements == NumElements)); }

We generally don't have this kind of predicate within the Type hierarchy. Convertibility information is generally captured in the type-compatibility routines in ASTContext (for C) and in the implicit conversions in Sema (for C++). And, this function isn't actually correct: casting should be looking at the canonical types to determine compatibility/equivalence and cv-qualifiers need to be taken into account.

Index: lib/AST/ASTContext.cpp
===================================================================
--- lib/AST/ASTContext.cpp	(revision 94011)
+++ lib/AST/ASTContext.cpp	(working copy)
@@ -1596,7 +1596,8 @@

 /// getVectorType - Return the unique reference to a vector type of
 /// the specified element type and size. VectorType must be a built-in type.
-QualType ASTContext::getVectorType(QualType vecType, unsigned NumElts) {
+QualType ASTContext::getVectorType(QualType vecType, unsigned NumElts,
+                                   bool IsAltiVec, bool IsPixel) {
   BuiltinType *baseType;
 
   baseType = dyn_cast<BuiltinType>(getCanonicalType(vecType).getTypePtr());
@@ -1604,7 +1605,8 @@

 
   // Check if we've already instantiated a vector of this type.
   llvm::FoldingSetNodeID ID;
-  VectorType::Profile(ID, vecType, NumElts, Type::Vector);
+  VectorType::Profile(ID, vecType, NumElts, Type::Vector,
+    IsAltiVec, IsPixel);
   void *InsertPos = 0;
   if (VectorType *VTP = VectorTypes.FindNodeOrInsertPos(ID, InsertPos))
     return QualType(VTP, 0);
@@ -1613,14 +1615,15 @@

   // so fill in the canonical type field.
   QualType Canonical;
   if (!vecType.isCanonical()) {
-    Canonical = getVectorType(getCanonicalType(vecType), NumElts);
+    Canonical = getVectorType(getCanonicalType(vecType),
+      NumElts, IsAltiVec, IsPixel);

Here, we're making the AltiVec and Pixel bits part of the canonical type, which means that, for example, the two typedefs below name different types:

	typedef vector unsigned int altivec_vector;
	typedef  __attribute__((vector_size(16))) unsigned int gcc_vector;

However, GCC seems to be treating the types as identical. Using those typedefs with the following program:

template<typename T, typename U>
struct is_same {
  static const bool value = false;
};

template<typename T>
struct is_same<T, T> {
  static const bool value = true;
};

int array0[is_same<altivec_vector, gcc_vector>::value? 1 : -1];

altivec_vector ***p1;
gcc_vector ***p2 = p1;
altivec_vector ***p3 = p2;

void f0(altivec_vector) { }
void f0(gcc_vector) { }

we get the results we would expect if altivec_vector and gcc_vector were just typedefs of the same type, meaning:

	- is_same<altivec_vector, gcc_vector>::value is true, indicating that GCC considers these types equivalent in C++
	- the pointer initializations work
	- the second definition of f0() is flagged as an error, since f0(altivec_vector) is already defined

This tells me that the AltiVec vector types are identical to the GCC vector types, which actually simplifies things considerably. It makes isImplicitCastable() unnecessary, since you don't need to cast between two identical types, and a lot of the other interactions between GCC and AltiVec vector types just work: there's no need to change the type-compatibility or type-conversion rules.

From a practical standpoint, it means that this:

   // so fill in the canonical type field.
   QualType Canonical;
   if (!vecType.isCanonical()) {
-    Canonical = getVectorType(getCanonicalType(vecType), NumElts);
+    Canonical = getVectorType(getCanonicalType(vecType),
+      NumElts, IsAltiVec, IsPixel);

should turn into:

   // so fill in the canonical type field.
   QualType Canonical;
   if (!vecType.isCanonical() || IsAltiVec || IsPixel) {
-    Canonical = getVectorType(getCanonicalType(vecType), NumElts);
+    Canonical = getVectorType(getCanonicalType(vecType),
+      NumElts, false, false);

so that the canonical type of an AltiVec vector type is the same as the equivalent GCC vector type. However, we still have the AltiVec and Pixel flags in the VectorType representation, so that we can print the type as the user wrote it.

Index: include/clang/Parse/DeclSpec.h
===================================================================
--- include/clang/Parse/DeclSpec.h	(revision 94011)
+++ include/clang/Parse/DeclSpec.h	(working copy)
@@ -153,6 +154,8 @@

   /*TSC*/unsigned TypeSpecComplex : 2;
   /*TSS*/unsigned TypeSpecSign : 2;
   /*TST*/unsigned TypeSpecType : 5;
+  bool TypeAltiVecVector : 1; // At present, these two flags
+  bool TypeAltiVecPixel : 1;  // only affect display output.
   bool TypeSpecOwned : 1;

If this is true, Parse/DeclSpec.h isn't the right place to put the comment. The parser is only responsible for reporting what it parsed to semantic analysis; the AST and Sema libraries are responsible for describing and coping with what the parser parsed. I think this comment belongs on the VectorType itself.

Index: lib/Sema/SemaOverload.cpp
===================================================================
--- lib/Sema/SemaOverload.cpp	(revision 94011)
+++ lib/Sema/SemaOverload.cpp	(working copy)

@@ -708,6 +708,12 @@

   } else if (IsNoReturnConversion(Context, FromType, ToType, FromType)) {
     // Treat a conversion that strips "noreturn" as an identity conversion.
     SCS.Second = ICK_NoReturn_Adjustment;
+  } else if (FromType->isVectorType() && ToType->isVectorType() &&
+      FromType->getAs<VectorType>()->isImplicitCastable(
+        ToType->getAs<VectorType>())) {
+    // Vector conversions
+    SCS.Second = ICK_Identity;
+    FromType = ToType.getUnqualifiedType();
   } else {
     // No second conversion required.
     SCS.Second = ICK_Identity;

This shouldn't be necessary when the canonical-types change above.

With the canonical-type changes above, this is ready to go in. Thanks!

	- Doug

On Jan 20, 2010, at 9:21 PM, John Thompson wrote:

> Sorry, I forgot to attach the updated patch.
> 
> On Wed, Jan 20, 2010 at 4:11 PM, John Thompson <john.thompson.jtsoftware at gmail.com> wrote:
> Hi, I haven't heard back about this patch regarding a first step towards AltiVec support.  Is it okay to check-in?
> 
> 
> On Tue, Jan 5, 2010 at 9:05 PM, John Thompson <john.thompson.jtsoftware at gmail.com> wrote:
> Sorry, I forgot to include the two updated altivec tests, plus update the tree before making the patch.  Please disregard the altivec1a.patch and look at this one instead.
>  
> Thanks.
>  
> -John
> 
> On Tue, Jan 5, 2010 at 6:33 PM, John Thompson <john.thompson.jtsoftware at gmail.com> wrote:
> Thanks, Doug.
>  
> Typo "preceeded".
> Fixed.
> 
>  
> I think this is right, but I want to check: are AltiVec vectors more like GCC's vectors (handled by VectorType) or more like OpenCL's vectors (handled by ExtVectorType)? OpenCL vectors are mainly GCC's vectors + the swizzling syntax like "myvec.xy".
>  
>  
> I checked the Altivec standard, and don't see anything like the swizzling syntax, so I think they're more like the GCC vectors.
> 
> It would be more efficient just to keep the IdentiferInfo * for the identifiers "vector" and "pixel" in the parser (only when AltiVec is enabled, of course), then do a pointer comparison rather than a string comparison. 
>  
> Good idea.  I've fixed this.
>  
> 
> +        const Token &nextToken = NextToken();
> 
> Tokens are small; it's best to just copy them, e.g.,
> 
>   Token nextToken = NextToken();
>  
>  
> Got it.
>  
> By changing the current token, we lose track of whether we saw "vector" or "__vector"; I guess it doesn't really matter. Still, wouldn't it be easier just to update the DeclSpec at this point?
>  
> Yep.
> 
>  
> At some point, we'll need more semantic checking here, e.g., to ignore "long long" and complain about the deprecation of "long", if I'm to believe
>  
> Yep.  I put in a warning for use of "long", and an error for use of "double".
>  
> http://publib.boulder.ibm.com/infocenter/macxhelp/v6v81/index.jsp?topic=/com.ibm.vacpp6m.doc/language/ref/clrc03altivec_types.htm
> 
> Or perhaps there's some actual AltiVec specification I could use to help understand this stuff better?
>  
> Here's the one I've been using: http://www.freescale.com/files/32bit/doc/ref_manual/ALTIVECPIM.pdf
>  
> It would probably be better to print with "__vector" and "__pixel", since those keywords are always available.
>  
> Got it.
>  
> This makes it so that AltiVec vectors are completely distinct types from equivalently-sized vectors (as declared with __attribute__((vector_size(blah))). If we want to be able to cast between AltiVec vectors and GCC-style vectors, we'll need to add type-compatibility logic (for C) and type-conversion logic (for C++). I don't know which way is correct... how does GCC handle compatibility between the various kinds of vectors? We'll need to follow its lead.
>  
> I took a crack at this in SemaOverload.cpp.
>  
> 
> Index: lib/Parse/ParseDecl.cpp
> ===================================================================
> --- lib/Parse/ParseDecl.cpp (revision 91204)
> +++ lib/Parse/ParseDecl.cpp (working copy)
> @@ -393,6 +393,12 @@
>    }
>  
>    if (AllowFunctionDefinitions && D.isFunctionDeclarator()) {
> +    // If attributes are present, parse them now.
> +    if (Tok.is(tok::kw___attribute)) {
> +      SourceLocation Loc;
> +      AttributeList *AttrList = ParseGNUAttributes(&Loc);
> +      D.AddAttributes(AttrList, Loc);
> +    }
> 
> This seems totally unrelated to AltiVec vectors?
>  
> Sorry.  Some code from my attribute position fix that hasn't gotten through yet.
> I pulled it for now.
>  
> pixel is an unsigned short? Weird.
>  
> Yeah, totally weird.
>  
> I've enclosed an updated patch.
>  
> -John
>  
> 
> 
> 
> -- 
> John Thompson
> John.Thompson.JTSoftware at gmail.com
> 
> 
> 
> 
> -- 
> John Thompson
> John.Thompson.JTSoftware at gmail.com
> 
> 
> 
> 
> -- 
> John Thompson
> John.Thompson.JTSoftware at gmail.com
> 
> <altivec1c.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100126/49a861f3/attachment.html>


More information about the cfe-commits mailing list