[OpenCL] Refactor out ReadPipe/WritePipe

[joey]

Summary

This patch to keep the pipe access qualifier inside PipeType itself looks cleaner overall than my previous approach.

Yaron had a post-commit comment, which lead me to re-think the original code.

[yaron.keren]

lib/AST/ASTContext.cpp

8258

I'm not too familar how PipeTypes should be merged, is it on purpose the process is not commutative,

ReadPipeType merged with WritePipeType is ReadPipeType

WritePipeType merged with ReadPipeType is WritePipeType

[joey]

Pipe types cannot be merged by ASTContext::mergeTypes.

[yaron.keren]

LGTM after fixing the inline comment

lib/Serialization/ASTReader.cpp

5804

return Context,getPipeType(ElementType, ReadOnly)

[joey]

Committed in r288332. Thanks Yaron!

include/clang/AST/ASTContext.h

class ASTContext : public RefCountedBase<ASTContext> {
   llvm::FoldingSet<PackExpansionType> PackExpansionTypes;
   mutable llvm::FoldingSet<ObjCObjectTypeImpl> ObjCObjectTypes;
   mutable llvm::FoldingSet<ObjCObjectPointerType> ObjCObjectPointerTypes;
   mutable llvm::FoldingSet<DependentUnaryTransformType>
     DependentUnaryTransformTypes;
   mutable llvm::FoldingSet<AutoType> AutoTypes;
   mutable llvm::FoldingSet<AtomicType> AtomicTypes;
   llvm::FoldingSet<AttributedType> AttributedTypes;
-  mutable llvm::FoldingSet<ReadPipeType> ReadPipeTypes;
-  mutable llvm::FoldingSet<WritePipeType> WritePipeTypes;
+  mutable llvm::FoldingSet<PipeType> PipeTypes;
 
   mutable llvm::FoldingSet<QualifiedTemplateName> QualifiedTemplateNames;
   mutable llvm::FoldingSet<DependentTemplateName> DependentTemplateNames;
   mutable llvm::FoldingSet<SubstTemplateTemplateParmStorage> 
     SubstTemplateTemplateParms;
   mutable llvm::ContextualFoldingSet<SubstTemplateTemplateParmPackStorage,
                                      ASTContext&> 
     SubstTemplateTemplateParmPacks;
#include "clang/Basic/OpenCLImageTypes.def"
   //===--------------------------------------------------------------------===//
 
 private:
   /// \brief Return a type with extended qualifiers.
   QualType getExtQualType(const Type *Base, Qualifiers Quals) const;
 
   QualType getTypeDeclTypeSlow(const TypeDecl *Decl) const;
 
+  QualType getPipeType(QualType T, bool ReadOnly) const;
+
 public:
   /// \brief Return the uniqued reference to the type for an address space
   /// qualified type with the specified type and address space.
   ///
   /// The resulting type has a union of the qualifiers from T and the address
   /// space. If T already has an address space specifier, it is silently
   /// replaced.
   QualType getAddrSpaceQualType(QualType T, unsigned AddressSpace) const;

include/clang/AST/Type.h

   }
   static bool classof(const Type *T) {
     return T->getTypeClass() == Atomic;
   }
 };
 
 /// PipeType - OpenCL20.
 class PipeType : public Type, public llvm::FoldingSetNode {
-protected:
   QualType ElementType;
   bool isRead;
 
   PipeType(QualType elemType, QualType CanonicalPtr, bool isRead) :
     Type(Pipe, CanonicalPtr, elemType->isDependentType(),
          elemType->isInstantiationDependentType(),
          elemType->isVariablyModifiedType(),
          elemType->containsUnexpandedParameterPack()),
     ElementType(elemType), isRead(isRead) {}
+  friend class ASTContext;  // ASTContext creates these.
 
 public:
   QualType getElementType() const { return ElementType; }
 
   bool isSugared() const { return false; }
 
   QualType desugar() const { return QualType(this, 0); }
 
   void Profile(llvm::FoldingSetNodeID &ID) {
-    Profile(ID, getElementType());
+    Profile(ID, getElementType(), isReadOnly());
   }
 
-  static void Profile(llvm::FoldingSetNodeID &ID, QualType T) {
+  static void Profile(llvm::FoldingSetNodeID &ID, QualType T, bool isRead) {
     ID.AddPointer(T.getAsOpaquePtr());
+    ID.AddBoolean(isRead);
   }
 
   static bool classof(const Type *T) {
     return T->getTypeClass() == Pipe;
   }
 
   bool isReadOnly() const { return isRead; }
 };
 
-class ReadPipeType : public PipeType {
-  ReadPipeType(QualType elemType, QualType CanonicalPtr) :
-    PipeType(elemType, CanonicalPtr, true) {}
-  friend class ASTContext;  // ASTContext creates these.
-};
-
-class WritePipeType : public PipeType {
-  WritePipeType(QualType elemType, QualType CanonicalPtr) :
-    PipeType(elemType, CanonicalPtr, false) {}
-  friend class ASTContext;  // ASTContext creates these.
-};
-
 /// A qualifier set is used to build a set of qualifiers.
 class QualifierCollector : public Qualifiers {
 public:
   QualifierCollector(Qualifiers Qs = Qualifiers()) : Qualifiers(Qs) {}
 
   /// Collect any qualifiers on the given type and return an
   /// unqualified type.  The qualifiers are assumed to be consistent
   /// with those already in the type.

include/clang/Serialization/ASTBitCodes.h

enum TypeCode {
       /// \brief A UnaryTransformType record.
       TYPE_UNARY_TRANSFORM       = 39,
       /// \brief An AtomicType record.
       TYPE_ATOMIC                = 40,
       /// \brief A DecayedType record.
       TYPE_DECAYED               = 41,
       /// \brief An AdjustedType record.
       TYPE_ADJUSTED              = 42,
-      /// \brief A ReadPipeType record.
-      TYPE_READ_PIPE             = 43,
+      /// \brief A PipeType record.
+      TYPE_PIPE                  = 43,
       /// \brief An ObjCTypeParamType record.
-      TYPE_OBJC_TYPE_PARAM       = 44,
-      /// \brief A WritePipeType record.
-      TYPE_WRITE_PIPE            = 45,
+      TYPE_OBJC_TYPE_PARAM       = 44
     };
 
     /// \brief The type IDs for special types constructed by semantic
     /// analysis.
     ///
     /// The constants in this enumeration are indices into the
     /// SPECIAL_TYPES record.
     enum SpecialTypeIDs {

lib/AST/ASTContext.cpp

   FunctionProtoType::ExtProtoInfo newEPI = EPI;
   new (FTP) FunctionProtoType(ResultTy, ArgArray, Canonical, newEPI);
   Types.push_back(FTP);
   if (!Unique)
     FunctionProtoTypes.InsertNode(FTP, InsertPos);
   return QualType(FTP, 0);
 }
 
-QualType ASTContext::getReadPipeType(QualType T) const {
+QualType ASTContext::getPipeType(QualType T, bool ReadOnly) const {
   llvm::FoldingSetNodeID ID;
-  ReadPipeType::Profile(ID, T);
+  PipeType::Profile(ID, T, ReadOnly);
 
   void *InsertPos = 0;
-  if (ReadPipeType *PT = ReadPipeTypes.FindNodeOrInsertPos(ID, InsertPos))
+  if (PipeType *PT = PipeTypes.FindNodeOrInsertPos(ID, InsertPos))
     return QualType(PT, 0);
 
   // If the pipe element type isn't canonical, this won't be a canonical type
   // either, so fill in the canonical type field.
   QualType Canonical;
   if (!T.isCanonical()) {
-    Canonical = getReadPipeType(getCanonicalType(T));
+    Canonical = getPipeType(getCanonicalType(T), ReadOnly);
 
     // Get the new insert position for the node we care about.
-    ReadPipeType *NewIP = ReadPipeTypes.FindNodeOrInsertPos(ID, InsertPos);
+    PipeType *NewIP = PipeTypes.FindNodeOrInsertPos(ID, InsertPos);
     assert(!NewIP && "Shouldn't be in the map!");
     (void)NewIP;
   }
-  ReadPipeType *New = new (*this, TypeAlignment) ReadPipeType(T, Canonical);
+  PipeType *New = new (*this, TypeAlignment) PipeType(T, Canonical, ReadOnly);
   Types.push_back(New);
-  ReadPipeTypes.InsertNode(New, InsertPos);
+  PipeTypes.InsertNode(New, InsertPos);
   return QualType(New, 0);
 }
 
-QualType ASTContext::getWritePipeType(QualType T) const {
-  llvm::FoldingSetNodeID ID;
-  WritePipeType::Profile(ID, T);
-
-  void *InsertPos = 0;
-  if (WritePipeType *PT = WritePipeTypes.FindNodeOrInsertPos(ID, InsertPos))
-    return QualType(PT, 0);
-
-  // If the pipe element type isn't canonical, this won't be a canonical type
-  // either, so fill in the canonical type field.
-  QualType Canonical;
-  if (!T.isCanonical()) {
-    Canonical = getWritePipeType(getCanonicalType(T));
+QualType ASTContext::getReadPipeType(QualType T) const {
+  return getPipeType(T, true);
+}
 
-    // Get the new insert position for the node we care about.
-    WritePipeType *NewIP = WritePipeTypes.FindNodeOrInsertPos(ID, InsertPos);
-    assert(!NewIP && "Shouldn't be in the map!");
-    (void)NewIP;
-  }
-  WritePipeType *New = new (*this, TypeAlignment) WritePipeType(T, Canonical);
-  Types.push_back(New);
-  WritePipeTypes.InsertNode(New, InsertPos);
-  return QualType(New, 0);
+QualType ASTContext::getWritePipeType(QualType T) const {
+  return getPipeType(T, false);
 }
 
 #ifndef NDEBUG
 static bool NeedsInjectedClassNameType(const RecordDecl *D) {
   if (!isa<CXXRecordDecl>(D)) return false;
   const CXXRecordDecl *RD = cast<CXXRecordDecl>(D);
   if (isa<ClassTemplatePartialSpecializationDecl>(RD))
     return true;

     if (canAssignObjCInterfaces(LHS->getAs<ObjCObjectPointerType>(),
                                 RHS->getAs<ObjCObjectPointerType>()))
       return LHS;
 
     return QualType();
   }
   case Type::Pipe:
   {
-    // Merge two pointer types, while trying to preserve typedef info
-    QualType LHSValue = LHS->getAs<PipeType>()->getElementType();
-    QualType RHSValue = RHS->getAs<PipeType>()->getElementType();
-    if (Unqualified) {
-      LHSValue = LHSValue.getUnqualifiedType();
-      RHSValue = RHSValue.getUnqualifiedType();
-    }
-    QualType ResultType = mergeTypes(LHSValue, RHSValue, false,
-                                     Unqualified);
-    if (ResultType.isNull()) return QualType();
-    if (getCanonicalType(LHSValue) == getCanonicalType(ResultType))
-      return LHS;
-    if (getCanonicalType(RHSValue) == getCanonicalType(ResultType))
-      return RHS;
-    return isa<ReadPipeType>(LHS) ? getReadPipeType(ResultType)
-                                  : getWritePipeType(ResultType);
+    assert(LHS != RHS &&
+           "Equivalent pipe types should have already been handled!");
+    return QualType();
   }
   }
 
   llvm_unreachable("Invalid Type::Class!");
 }
 
 bool ASTContext::doFunctionTypesMatchOnExtParameterInfos(
                    const FunctionProtoType *firstFnType,
                    const FunctionProtoType *secondFnType) {
   // Fast path: if the first type doesn't have ext parameter infos,
   // we match if and only if they second type also doesn't have them.
   if (!firstFnType->hasExtParameterInfos())
     return !secondFnType->hasExtParameterInfos();
 
   // Otherwise, we can only match if the second type has them.
   if (!secondFnType->hasExtParameterInfos())
     return false;
 
   auto firstEPI = firstFnType->getExtParameterInfos();
   auto secondEPI = secondFnType->getExtParameterInfos();

lib/Serialization/ASTReader.cpp

     if (Record.size() != 1) {
       Error("Incorrect encoding of atomic type");
       return QualType();
     }
     QualType ValueType = readType(*Loc.F, Record, Idx);
     return Context.getAtomicType(ValueType);
   }
 
-  case TYPE_READ_PIPE: {
-    if (Record.size() != 1) {
+  case TYPE_PIPE: {
+    if (Record.size() != 2) {
       Error("Incorrect encoding of pipe type");
       return QualType();
     }
 
     // Reading the pipe element type.
     QualType ElementType = readType(*Loc.F, Record, Idx);
-    return Context.getReadPipeType(ElementType);
+    unsigned ReadOnly = Record[1];
+    if (ReadOnly)
+      return Context.getReadPipeType(ElementType);
+    else
+      return Context.getWritePipeType(ElementType);
   }
 
-  case TYPE_WRITE_PIPE: {
-    if (Record.size() != 1) {
-      Error("Incorrect encoding of pipe type");
-      return QualType();
-    }
-
-    // Reading the pipe element type.
-    QualType ElementType = readType(*Loc.F, Record, Idx);
-    return Context.getWritePipeType(ElementType);
-  }
   }
   llvm_unreachable("Invalid TypeCode!");
 }
 
 void ASTReader::readExceptionSpec(ModuleFile &ModuleFile,
                                   SmallVectorImpl<QualType> &Exceptions,
                                   FunctionProtoType::ExceptionSpecInfo &ESI,
                                   const RecordData &Record, unsigned &Idx) {

lib/Serialization/ASTWriter.cpp

 ASTTypeWriter::VisitAtomicType(const AtomicType *T) {
   Record.AddTypeRef(T->getValueType());
   Code = TYPE_ATOMIC;
 }
 
 void
 ASTTypeWriter::VisitPipeType(const PipeType *T) {
   Record.AddTypeRef(T->getElementType());
-  if (T->isReadOnly())
-    Code = TYPE_READ_PIPE;
-  else
-    Code = TYPE_WRITE_PIPE;
+  Record.push_back(T->isReadOnly());
+  Code = TYPE_PIPE;
 }
 
 namespace {
 
 class TypeLocWriter : public TypeLocVisitor<TypeLocWriter> {
   ASTRecordWriter &Record;
 
 public:

test/SemaOpenCL/invalid-pipes-cl2.0.cl

void test5(pipe int p) {
   p+p; // expected-error{{invalid operands to binary expression ('read_only pipe int' and 'read_only pipe int')}}
   p=p; // expected-error{{invalid operands to binary expression ('read_only pipe int' and 'read_only pipe int')}}
   &p; // expected-error{{invalid argument type 'read_only pipe int' to unary expression}}
   *p; // expected-error{{invalid argument type 'read_only pipe int' to unary expression}}
 }
 
 typedef pipe int pipe_int_t;
 pipe_int_t test6() {} // expected-error{{declaring function return value of type 'pipe_int_t' (aka 'read_only pipe int') is not allowed}}
+
+// Tests ASTContext::mergeTypes rejects this.
+int f(pipe int x, int y);
+int f(x, y)
+pipe short x;
+int y;
+{
+    return y;
+}