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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
