[Patch] OpenCL kernel-arg-info
Tanya Lattner
lattner at apple.com
Wed Mar 20 17:49:23 PDT 2013
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" <guy.benyei at intel.com> 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:lattner at apple.com]
> Sent: Wednesday, February 20, 2013 03:03
> To: Benyei, Guy
> Cc: Pekka J??skel?inen; cfe-commits at cs.uiuc.edu
> Subject: Re: [Patch] OpenCL kernel-arg-info
>
>
> On Feb 18, 2013, at 4:00 AM, "Benyei, Guy" <guy.benyei at intel.com> 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:pekka.jaaskelainen at tut.fi]
>> Sent: Sunday, February 17, 2013 18:23
>> To: Benyei, Guy
>> Cc: cfe-commits at cs.uiuc.edu
>> 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
>> cfe-commits at cs.uiuc.edu
>> 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>
More information about the cfe-commits
mailing list