[OpenCL] Fix pipe type printing in arg info metadata

[Anastasia]

Summary

Pipe element type spelling for arg info metadata should follow the same behavior as normal type spelling. We should not return the canonical type.

This patch also removed type handling duplication.

[stuart]

This looks like a really good cleanup, in addition to fixing the metadata for pipes.

Minor nit: "matadata" -> "metadata" in the description, and there's a StringRef construction that could be sunk.

clang/lib/CodeGen/CodeGenModule.cpp

1496–1507

This seems far cleaner than the code it replaces!

1499

Would it be better to sink this into the if (Ty.isCanonical()) block?

[mantognini]

Nice refactoring & fix! LGTM. I suppose Stuart's comment about moving typeNameRef can be addressed when pushing.

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::GenOpenCLArgMetadata(llvm::Function *Fn,
   SmallVector<llvm::Metadata *, 8> argNames;
 
   if (FD && CGF)
     for (unsigned i = 0, e = FD->getNumParams(); i != e; ++i) {
       const ParmVarDecl *parm = FD->getParamDecl(i);
       QualType ty = parm->getType();
       std::string typeQuals;
 
+      // Get image and pipe access qualifier:
+      if (ty->isImageType() || ty->isPipeType()) {
+        const Decl *PDecl = parm;
+        if (auto *TD = dyn_cast<TypedefType>(ty))
+          PDecl = TD->getDecl();
+        const OpenCLAccessAttr *A = PDecl->getAttr<OpenCLAccessAttr>();
+        if (A && A->isWriteOnly())
+          accessQuals.push_back(llvm::MDString::get(VMContext, "write_only"));
+        else if (A && A->isReadWrite())
+          accessQuals.push_back(llvm::MDString::get(VMContext, "read_write"));
+        else
+          accessQuals.push_back(llvm::MDString::get(VMContext, "read_only"));
+      } else
+        accessQuals.push_back(llvm::MDString::get(VMContext, "none"));
+
+      // Get argument name.
+      argNames.push_back(llvm::MDString::get(VMContext, parm->getName()));
+
+      auto getTypeSpelling = [&](QualType Ty) {
+        auto typeName = Ty.getUnqualifiedType().getAsString(Policy);
+
+        if (Ty.isCanonical()) {
+          StringRef typeNameRef = typeName;
+          // Turn "unsigned type" to "utype"
+          if (typeNameRef.consume_front("unsigned "))
+            return std::string("u") + typeNameRef.str();
+        }
+
+        return typeName;
+      };
+
       if (ty->isPointerType()) {
         QualType pointeeTy = ty->getPointeeType();
 
         // Get address qualifier.
         addressQuals.push_back(
             llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(
                 ArgInfoAddressSpace(pointeeTy.getAddressSpace()))));
 
         // Get argument type name.
-        std::string typeName =
-            pointeeTy.getUnqualifiedType().getAsString(Policy) + "*";
-
-        // Turn "unsigned type" to "utype"
-        std::string::size_type pos = typeName.find("unsigned");
-        if (pointeeTy.isCanonical() && pos != std::string::npos)
-          typeName.erase(pos + 1, 8);
-
-        argTypeNames.push_back(llvm::MDString::get(VMContext, typeName));
-
+        std::string typeName = getTypeSpelling(pointeeTy) + "*";
         std::string baseTypeName =
-            pointeeTy.getUnqualifiedType().getCanonicalType().getAsString(
-                Policy) +
-            "*";
-
-        // Turn "unsigned type" to "utype"
-        pos = baseTypeName.find("unsigned");
-        if (pos != std::string::npos)
-          baseTypeName.erase(pos + 1, 8);
-
+            getTypeSpelling(pointeeTy.getCanonicalType()) + "*";
+        argTypeNames.push_back(llvm::MDString::get(VMContext, typeName));
         argBaseTypeNames.push_back(
             llvm::MDString::get(VMContext, baseTypeName));
 
         // Get argument type qualifiers:
         if (ty.isRestrictQualified())
           typeQuals = "restrict";
         if (pointeeTy.isConstQualified() ||
             (pointeeTy.getAddressSpace() == LangAS::opencl_constant))
           typeQuals += typeQuals.empty() ? "const" : " const";
         if (pointeeTy.isVolatileQualified())
           typeQuals += typeQuals.empty() ? "volatile" : " volatile";
       } else {
         uint32_t AddrSpc = 0;
         bool isPipe = ty->isPipeType();
         if (ty->isImageType() || isPipe)
           AddrSpc = ArgInfoAddressSpace(LangAS::opencl_global);
 
         addressQuals.push_back(
             llvm::ConstantAsMetadata::get(CGF->Builder.getInt32(AddrSpc)));
 
         // Get argument type name.
-        std::string typeName;
-        if (isPipe)
-          typeName = ty.getCanonicalType()
-                         ->castAs<PipeType>()
-                         ->getElementType()
-                         .getAsString(Policy);
-        else
-          typeName = ty.getUnqualifiedType().getAsString(Policy);
-
-        // Turn "unsigned type" to "utype"
-        std::string::size_type pos = typeName.find("unsigned");
-        if (ty.isCanonical() && pos != std::string::npos)
-          typeName.erase(pos + 1, 8);
-
-        std::string baseTypeName;
-        if (isPipe)
-          baseTypeName = ty.getCanonicalType()
-                             ->castAs<PipeType>()
-                             ->getElementType()
-                             .getCanonicalType()
-                             .getAsString(Policy);
-        else
-          baseTypeName =
-              ty.getUnqualifiedType().getCanonicalType().getAsString(Policy);
+        ty = isPipe ? ty->castAs<PipeType>()->getElementType() : ty;
+        std::string typeName = getTypeSpelling(ty);
+        std::string baseTypeName = getTypeSpelling(ty.getCanonicalType());
 
         // Remove access qualifiers on images
         // (as they are inseparable from type in clang implementation,
         // but OpenCL spec provides a special query to get access qualifier
         // via clGetKernelArgInfo with CL_KERNEL_ARG_ACCESS_QUALIFIER):
         if (ty->isImageType()) {
           removeImageAccessQualifier(typeName);
           removeImageAccessQualifier(baseTypeName);
         }
 
         argTypeNames.push_back(llvm::MDString::get(VMContext, typeName));
-
-        // Turn "unsigned type" to "utype"
-        pos = baseTypeName.find("unsigned");
-        if (pos != std::string::npos)
-          baseTypeName.erase(pos + 1, 8);
-
         argBaseTypeNames.push_back(
             llvm::MDString::get(VMContext, baseTypeName));
 
         if (isPipe)
           typeQuals = "pipe";
       }
-
       argTypeQuals.push_back(llvm::MDString::get(VMContext, typeQuals));
-
-      // Get image and pipe access qualifier:
-      if (ty->isImageType() || ty->isPipeType()) {
-        const Decl *PDecl = parm;
-        if (auto *TD = dyn_cast<TypedefType>(ty))
-          PDecl = TD->getDecl();
-        const OpenCLAccessAttr *A = PDecl->getAttr<OpenCLAccessAttr>();
-        if (A && A->isWriteOnly())
-          accessQuals.push_back(llvm::MDString::get(VMContext, "write_only"));
-        else if (A && A->isReadWrite())
-          accessQuals.push_back(llvm::MDString::get(VMContext, "read_write"));
-        else
-          accessQuals.push_back(llvm::MDString::get(VMContext, "read_only"));
-      } else
-        accessQuals.push_back(llvm::MDString::get(VMContext, "none"));
-
-      // Get argument name.
-      argNames.push_back(llvm::MDString::get(VMContext, parm->getName()));
     }
 
   Fn->setMetadata("kernel_arg_addr_space",
                   llvm::MDNode::get(VMContext, addressQuals));
   Fn->setMetadata("kernel_arg_access_qual",
                   llvm::MDNode::get(VMContext, accessQuals));
   Fn->setMetadata("kernel_arg_type",
                   llvm::MDNode::get(VMContext, argTypeNames));

clang/test/CodeGenOpenCL/kernel-arg-info.cl

 // CHECK: !kernel_arg_type ![[MD61:[0-9]+]]
 // ARGINFO: !kernel_arg_name ![[MD62:[0-9]+]]
 
 typedef read_only  image1d_t ROImage;
 typedef write_only image1d_t WOImage;
 typedef read_write image1d_t RWImage;
 kernel void foo7(ROImage ro, WOImage wo, RWImage rw) {
 }
+
 // CHECK: define{{.*}} spir_kernel void @foo7{{[^!]+}}
 // CHECK: !kernel_arg_addr_space ![[MD71:[0-9]+]]
 // CHECK: !kernel_arg_access_qual ![[MD72:[0-9]+]]
 // CHECK: !kernel_arg_type ![[MD73:[0-9]+]]
 // CHECK: !kernel_arg_base_type ![[MD74:[0-9]+]]
 // CHECK: !kernel_arg_type_qual ![[MD75:[0-9]+]]
 // CHECK-NOT: !kernel_arg_name
 // ARGINFO: !kernel_arg_name ![[MD76:[0-9]+]]
 
+typedef unsigned char uchar;
+typedef uchar uchar2 __attribute__((ext_vector_type(2)));
+kernel void foo8(pipe int p1, pipe uchar p2, pipe uchar2 p3, const pipe uchar p4, write_only pipe uchar p5) {}
+// CHECK: define{{.*}} spir_kernel void @foo8{{[^!]+}}
+// CHECK: !kernel_arg_addr_space ![[PIPE_AS_QUAL:[0-9]+]]
+// CHECK: !kernel_arg_access_qual ![[PIPE_ACCESS_QUAL:[0-9]+]]
+// CHECK: !kernel_arg_type ![[PIPE_TY:[0-9]+]]
+// CHECK: !kernel_arg_base_type ![[PIPE_BASE_TY:[0-9]+]]
+// CHECK: !kernel_arg_type_qual ![[PIPE_QUAL:[0-9]+]]
+// CHECK-NOT: !kernel_arg_name
+// ARGINFO: !kernel_arg_name ![[PIPE_ARG_NAMES:[0-9]+]]
+
 // CHECK: ![[MD11]] = !{i32 1, i32 1, i32 1, i32 1, i32 2, i32 2, i32 1, i32 1, i32 1, i32 1, i32 3, i32 3, i32 3, i32 3, i32 3, i32 3, i32 3, i32 3, i32 0, i32 0, i32 0, i32 0}
 // CHECK: ![[MD12]] = !{!"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none", !"none"}
 // CHECK: ![[MD13]] = !{!"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int*", !"int", !"int", !"int", !"int"}
 // CHECK: ![[MD14]] = !{!"", !"restrict", !"const", !"restrict const", !"const", !"restrict const", !"const volatile", !"restrict const volatile", !"volatile", !"restrict volatile", !"", !"restrict", !"const", !"restrict const", !"const volatile", !"restrict const volatile", !"volatile", !"restrict volatile", !"", !"", !"", !""}
 // ARGINFO: ![[MD15]] = !{!"globalintp", !"globalintrestrictp", !"globalconstintp", !"globalconstintrestrictp", !"constantintp", !"constantintrestrictp", !"globalconstvolatileintp", !"globalconstvolatileintrestrictp", !"globalvolatileintp", !"globalvolatileintrestrictp", !"localintp", !"localintrestrictp", !"localconstintp", !"localconstintrestrictp", !"localconstvolatileintp", !"localconstvolatileintrestrictp", !"localvolatileintp", !"localvolatileintrestrictp", !"X", !"constint", !"constvolatileint", !"volatileint"}
 // CHECK: ![[MD21]] = !{i32 1, i32 1, i32 1, i32 1}
 // CHECK: ![[MD22]] = !{!"read_only", !"read_only", !"write_only", !"read_write"}
 // CHECK: ![[MD23]] = !{!"image1d_t", !"image2d_t", !"image2d_array_t", !"image1d_t"}

 // CHECK: ![[MD61]] = !{!"char16*"}
 // ARGINFO: ![[MD62]] = !{!"arg"}
 // CHECK: ![[MD71]] = !{i32 1, i32 1, i32 1}
 // CHECK: ![[MD72]] = !{!"read_only", !"write_only", !"read_write"}
 // CHECK: ![[MD73]] = !{!"ROImage", !"WOImage", !"RWImage"}
 // CHECK: ![[MD74]] = !{!"image1d_t", !"image1d_t", !"image1d_t"}
 // CHECK: ![[MD75]] = !{!"", !"", !""}
 // ARGINFO: ![[MD76]] = !{!"ro", !"wo", !"rw"}
-
+// CHECK: ![[PIPE_AS_QUAL]] = !{i32 1, i32 1, i32 1, i32 1, i32 1}
+// CHECK: ![[PIPE_ACCESS_QUAL]] = !{!"read_only", !"read_only", !"read_only", !"read_only", !"write_only"}
+// CHECK: ![[PIPE_TY]] = !{!"int", !"uchar", !"uchar2", !"uchar", !"uchar"}
+// CHECK: ![[PIPE_BASE_TY]] = !{!"int", !"uchar", !"uchar __attribute__((ext_vector_type(2)))", !"uchar", !"uchar"}
+// CHECK: ![[PIPE_QUAL]] = !{!"pipe", !"pipe", !"pipe", !"pipe", !"pipe"}
+// ARGINFO: ![[PIPE_ARG_NAMES]] = !{!"p1", !"p2", !"p3", !"p4", !"p5"}