I think this is fine to commit. I may have some modifications later if I ever get to merging my changes back that are related.
Is the change to this related though? include/clang/Basic/Attr.td Separate commit? -Tanya On Feb 21, 2013, at 9:23 AM, "Benyei, Guy" <[email protected]> wrote: > Hi Tanya, > > Attached an updated patch with fixes for your comment. > > "In addition, I believe that for pointers that the qualifier's list should > include the qualifiers of the pointer and the pointee. I will need to double > check which conformance test this is for, but I had to add this to pass > conformance." > > AFAIK, the const and volatile qualifiers go to the pointee, and the restrict > qualifier belongs to the pointer. That's how our implementation works, and > it's fully conformant. This is also what I did in this patch. > > I also know about the "unsigned" issue, but as you said, "The consumer of > said metadata can transform it into anything they wish." Anyhow, I've added a > fix for that, and I also agree, that adding the missing types in Clang is a > better solution. > > > Please review > Thanks > Guy > > -----Original Message----- > From: Tanya Lattner [mailto:[email protected]] > Sent: Wednesday, February 20, 2013 03:03 > To: Benyei, Guy > Cc: Pekka J??skel?inen; [email protected] > Subject: Re: [Patch] OpenCL kernel-arg-info > > > On Feb 18, 2013, at 4:00 AM, "Benyei, Guy" <[email protected]> wrote: > >> Pekka, >> Thanks for the prompt review. >> >> Attached an updated patch. Added a header file with the kKernelArgInfo >> specific enumerations. > > My comments: > > - I really would like to avoid having a header file as it doesn't seem > necessary at all. The kernel arg info doesn't need to use arbitrary numbers. > The consumer of said metadata can transform it into anything they wish. See > below for my comments on this. > > // Main MDNode holding all the kernel arg metadata for a specific kernel. > + SmallVector<llvm::Value*, 8> argInfo; > + argInfo.push_back(llvm::MDString::get(Context, "cl-kernel-arg-info")); > > This is not needed at all. > > // MDNode for the kernel argument address space qualifiers. > + SmallVector<llvm::Value*, 8> addressQuals; > + addressQuals.push_back(llvm::MDString::get(Context, > + "address_qualifiers")); > + > + // MDNode for the kernel argument access qualifiers (images only). > + SmallVector<llvm::Value*, 8> accessQuals; > + accessQuals.push_back(llvm::MDString::get(Context, > + "access_qualifiers")); > + > + // MDNode for the kernel argument type names. > + SmallVector<llvm::Value*, 8> argTypeNames; > + argTypeNames.push_back(llvm::MDString::get(Context, > + "arg_type_names")); > + > + // MDNode for the kernel argument type qualifiers. > + SmallVector<llvm::Value*, 8> argTypeQuals; > + argTypeQuals.push_back(llvm::MDString::get(Context, > + "arg_type_qualifiers")); > + > // MDNode for the kernel argument names. > SmallVector<llvm::Value*, 8> argNames; > - argNames.push_back(llvm::MDString::get(Context, "kernel_arg_name")); > + argNames.push_back(llvm::MDString::get(Context, "arg_names")); > > I would prefer that these be changed to: > address_qualifiers -> kernel_arg_addr_space access_qualifier -> > kernel_arg_access_qual arg_type_names -> kernel_arg_type arg_type_qualifiers > - > kernel_arg_type_qual > > This matches the type "kernel_arg_*" that was agreed upon when I first > committed the name patch. > > for (unsigned i = 0, e = FD->getNumParams(); i != e; ++i) { > const ParmVarDecl *parm = FD->getParamDecl(i); > + QualType ty = parm->getType(); > + int typeQuals = CLKAITQ_none; > > + if (ty->isPointerType()) { > + QualType pointeeTy = ty->getPointeeType(); > + > + // Get address qualifier. > + if (pointeeTy.getAddressSpace() == 0) > + addressQuals.push_back(Builder.getInt32(CLKAIAS_private)); > + else if (ty->getPointeeType().getAddressSpace() == > + LangAS::opencl_global) > + addressQuals.push_back(Builder.getInt32(CLKAIAS_global)); > + else if (ty->getPointeeType().getAddressSpace() == > + LangAS::opencl_constant) { > + addressQuals.push_back(Builder.getInt32(CLKAIAS_constant)); > + typeQuals |= 1; > + } else if (ty->getPointeeType().getAddressSpace() == > + LangAS::opencl_local) > + addressQuals.push_back(Builder.getInt32(CLKAIAS_local)); > + > > You should not be using set numbers here but using the Target Address Space > map. > > + // Get argument type name. > + argTypeNames.push_back( > + llvm::MDString::get(Context, > + > + pointeeTy.getUnqualifiedType().getAsString() + "*")); > + > + // Get argument type qualifiers: > + if(ty.isRestrictQualified()) > + typeQuals |= CLKAITQ_restrict; > + if(pointeeTy.isConstQualified()) > + typeQuals |= CLKAITQ_const; > + if(pointeeTy.isVolatileQualified()) > + typeQuals |= CLKAITQ_volatile; > + > + argTypeQuals.push_back(Builder.getInt32(typeQuals)); > + } else { > + addressQuals.push_back(Builder.getInt32(0)); > + > + // Get argument type name. > + argTypeNames.push_back( > + llvm::MDString::get(Context, > + ty.getUnqualifiedType().getAsString())); > + > > This is not correct and will not pass conformance. There are many little > details here such as unsigned int should be uint, all the vector types, etc. > I plan to address the missing types in Clang very very soon which will make > actually implementing type name possible as the types will be "standard" > across implementations. If you want to leave this out for now, then I will > take care of it since I have already implemented this for us. > > + // Get argument type qualifiers: > + if(ty.isConstQualified()) > + typeQuals |= CLKAITQ_const; > + if(ty.isVolatileQualified()) > + typeQuals |= CLKAITQ_volatile; > + > + argTypeQuals.push_back(Builder.getInt32(typeQuals)); > + } > > I recommend using strings to represent this data. Then you have no arbitrary > numbers. If the consumer wants to access this information and process it into > another format, than they can. We use "const", "restrict", "volatile" and > "none" in our implementation. > > In addition, I believe that for pointers that the qualifier's list should > include the qualifiers of the pointer and the pointee. I will need to double > check which conformance test this is for, but I had to add this to pass > conformance. > > + > + // Get image access qualifier: > + if (ty->isImageType()) { > + if (parm->hasAttr<OpenCLImageAccessAttr>() && > + parm->getAttr<OpenCLImageAccessAttr>()->getAccess() == > CLIA_write_only) > + accessQuals.push_back(Builder.getInt32(CLKAIAQ_write_only)); > + else > + accessQuals.push_back(Builder.getInt32(CLKAIAQ_read_only)); > + } else > + accessQuals.push_back(Builder.getInt32(CLKAIAQ_none)); > + > // Get argument name. > argNames.push_back(llvm::MDString::get(Context, parm->getName())); > + } > > I would use strings here too and avoid using arbitrary numbers to represent > the access qualifiers. Also removes the need for the header file. > > - } > + argInfo.push_back(llvm::MDNode::get(Context, addressQuals)); > + argInfo.push_back(llvm::MDNode::get(Context, accessQuals)); > + argInfo.push_back(llvm::MDNode::get(Context, argTypeNames)); > + argInfo.push_back(llvm::MDNode::get(Context, argTypeQuals)); > + argInfo.push_back(llvm::MDNode::get(Context, argNames)); > + > // Add MDNode to the list of all metadata. > - kernelMDArgs.push_back(llvm::MDNode::get(Context, argNames)); > + kernelMDArgs.push_back(llvm::MDNode::get(Context, argInfo)); > > This level of nesting isn't needed. Just attach everything to kernelMDArgs. > > -Tanya > > > > > > > > > > > > > > > >> >> Please review. >> >> Thanks >> Guy >> >> -----Original Message----- >> From: Pekka Jääskeläinen [mailto:[email protected]] >> Sent: Sunday, February 17, 2013 18:23 >> To: Benyei, Guy >> Cc: [email protected] >> Subject: Re: [Patch] OpenCL kernel-arg-info >> >> Hi, >> >> You use "magic numbers" for the different address space ids. >> Similarly the type qualifier bit masks could use some named consts, IMO. >> >> These are from the SPIR specs, but still, using some sort of named >> constant/enum from a header file would clean it up. >> There could be a header for the SPIR-specific constants? >> >> The consumers for this metadata (at least the OpenCL >> clGetKernelArgInfo() implementations) need to refer to these numbers too. >> >> + if(ty.isRestrictQualified()) typeQuals |= 2; >> + if(pointeeTy.isConstQualified()) typeQuals |= 1; >> + if(pointeeTy.isVolatileQualified()) typeQuals |= 4; >> >> Some white space issues. >> >> On 02/17/2013 04:49 PM, Benyei, Guy wrote: >>> Please review. >> >> -- >> --Pekka >> >> --------------------------------------------------------------------- >> Intel Israel (74) Limited >> >> This e-mail and any attachments may contain confidential material for >> the sole use of the intended recipient(s). Any review or distribution >> by others is strictly prohibited. If you are not the intended >> recipient, please contact the sender and delete all copies. >> <opencl_arg_info2.patch>______________________________________________ >> _ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > --------------------------------------------------------------------- > Intel Israel (74) Limited > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > <opencl_arg_info3.patch> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
