[PATCH] D14441: [OpenCL] Pipe types support.

[Pekka Jääskeläinen via cfe-commits]

pekka.jaaskelainen requested changes to this revision.
pekka.jaaskelainen added a comment.
This revision now requires changes to proceed.

Only small issues found, plus it could use some more test cases.


================
Comment at: include/clang/AST/TypeLoc.h:2049
@@ +2048,3 @@
+  SourceRange getLocalSourceRange() const {
+    return SourceRange( getKWLoc() );
+  }
----------------
Extra white spaces.

================
Comment at: include/clang/Sema/DeclSpec.h:1425
@@ -1415,1 +1424,3 @@
 
+    struct PipeTypeInfo : TypeInfoCommon {
+    /// The access writes.
----------------
Indent error.

================
Comment at: include/clang/Sema/DeclSpec.h:1426
@@ +1425,3 @@
+    struct PipeTypeInfo : TypeInfoCommon {
+    /// The access writes.
+    unsigned AccessWrites : 3;
----------------
I think this needs a better comment than duplicating the variable name. What is the purpose of the variable?

================
Comment at: include/clang/Sema/DeclSpec.h:1554
@@ +1553,3 @@
+  static DeclaratorChunk getPipe(unsigned TypeQuals,
+                                          SourceLocation Loc) {
+    DeclaratorChunk I;
----------------
Indent issue.

================
Comment at: include/clang/Serialization/ASTBitCodes.h:911
@@ +910,3 @@
+      TYPE_ADJUSTED              = 42,
+      /// \brief An PipeType record.
+      TYPE_PIPE                  = 43
----------------
A

================
Comment at: lib/AST/ASTContext.cpp:3121
@@ +3120,3 @@
+/// getPipeType - Return pipe type for the specified type.
+QualType ASTContext::getPipeType(QualType T ) const {
+  // Unique pointers, to guarantee there is only one pointer of a particular
----------------
White space errror.

================
Comment at: lib/AST/ASTContext.cpp:3141
@@ +3140,3 @@
+  }
+  PipeType *New = new (*this, TypeAlignment) PipeType(T, Canonical);
+  Types.push_back(New);
----------------
Should we assign T to 'Canonical' if it already was Canonical or is this intentional somehow?

================
Comment at: lib/AST/ItaniumMangle.cpp:2672
@@ +2671,3 @@
+  // <type> ::= U <source-name> <type>	# vendor extended type qualifier
+  // (Until there's a standardized mangling...)
+  Out << "8ocl_pipe";
----------------
This seems to conform to SPIR 2.0, should we mention it here?

================
Comment at: lib/Parse/ParseDecl.cpp:4954
@@ +4953,3 @@
+
+  if ( D.getDeclSpec().isTypeSpecPipe() ) {
+    DeclSpec &DS = D.getMutableDeclSpec();
----------------
WS issues.

================
Comment at: lib/Parse/ParseDecl.cpp:4958
@@ +4957,3 @@
+      D.AddTypeInfo(DeclaratorChunk::getPipe(DS.getTypeQualifiers(),
+                                           DS.getPipeLoc()),
+                  DS.getAttributes(),
----------------
Indent issues.

================
Comment at: lib/Sema/DeclSpec.cpp:729
@@ +728,3 @@
+
+  if ( TypeSpecType != TST_unspecified ) {
+    PrevSpec = DeclSpec::getSpecifierName((TST) TypeSpecType, Policy);
----------------
WS issues.

================
Comment at: lib/Sema/DeclSpec.cpp:735
@@ +734,3 @@
+
+  if ( isPipe ) {
+    TypeSpecPipe = TSP_pipe;
----------------
ditto

================
Comment at: lib/Sema/SemaType.cpp:1570
@@ -1564,3 +1569,3 @@
   // attributes are pushed around.
-  if (AttributeList *attrs = DS.getAttributes().getList())
-    processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
+  if ( !DS.isTypeSpecPipe() ) {         // pipe attributes will be handled later ( at GetFullTypeForDeclarator )
+    if ( AttributeList *attrs = DS.getAttributes().getList())
----------------
WS issues and an overlong line.

================
Comment at: lib/Sema/SemaType.cpp:1571
@@ +1570,3 @@
+  if ( !DS.isTypeSpecPipe() ) {         // pipe attributes will be handled later ( at GetFullTypeForDeclarator )
+    if ( AttributeList *attrs = DS.getAttributes().getList())
+      processTypeAttrs(state, Result, TAL_DeclSpec, attrs);
----------------
WS issues.

================
Comment at: lib/Sema/SemaType.cpp:1945
@@ +1944,3 @@
+QualType Sema::BuildPipeType(QualType T,
+                                SourceLocation Loc) {
+  assert(!T->isObjCObjectType() && "Should build ObjCObjectPointerType");
----------------
Indent

================
Comment at: lib/Sema/TreeTransform.h:5254
@@ +5253,3 @@
+QualType TreeTransform<Derived>::TransformPipeType(TypeLocBuilder &TLB,
+                                                     PipeTypeLoc TL) {
+  QualType ValueType = getDerived().TransformType(TLB, TL.getValueLoc());
----------------
Indent

================
Comment at: test/CodeGenOpenCL/pipe_types.cl:1
@@ +1,2 @@
+// RUN: %clang_cc1 -emit-llvm -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+
----------------
Can you add a couple of more checks? For example:
- the behavior when using CL1.2
- when the inner type of the pipe has a 'const' or similar qualifier
- It can only by a function arg. What if it's a local variable?



http://reviews.llvm.org/D14441