Hi Tanya,
The Attr.td part is simply enabling the image access qualifier to be saved in
the AST as attribute - it is needed, so we can retrieve this information in the
CodeGen and emit the related metadata. We need it as a part of this commit.
This is of course not the full solution, since it doesn't really implement the
access qualifier.
It's also clear, that this metadata part will be updated following any patch
that affects the implementation of OpenCL features.
Thanks
Guy
-----Original Message-----
From: Tanya Lattner [mailto:[email protected]]
Sent: Thursday, March 21, 2013 02:49
To: Benyei, Guy
Cc: Pekka J??skel?inen; [email protected]
Subject: Re: [Patch] OpenCL kernel-arg-info
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>
---------------------------------------------------------------------
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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
