PDA

View Full Version : Feedback and errata for the 1.0.29 spec



rillian
01-08-2009, 06:03 PM
This thread is for poining out typos, inconsistencies, and general points which could use clarification in the OpenCL 1.0 spec, revision 29.

Post your errors and issues here as an aid to mutual understanding, interpreting it compatibly, and hopefully assisting the editors with the next revision.

rillian
01-08-2009, 06:33 PM
Notes I've made so far. First, a few simple corrections:


Section 4.2 "The number of OpenCL devices returned is the mininum of value specified" should be "the minimum of the value..." (Missing definite article.)
[/*:m:168j75vi]
Section 4.3 clCreateContext() is described as returning CL_INVALID_DEVICE_LIST which isn't defined in the Khronos header. Should be CL_INVALID_DEVICE?
[/*:m:168j75vi]
Section 5.1 The entry on clCreateCommandQueue() says the device parameter may be in the list of devices specified when creating the corresponding context "or have the same device type as device type specified when context is created using clCreateContextFromType." Should be "or have the same device type as the device type specified when the context..." (Two missing definite articles.)
[/*:m:168j75vi]
Section 5.5.3 clGetKernelWorkGroupInfo() is defined as taking a 'cl_device' in the second argument. Should be 'cl_device_id'.[/*:m:168j75vi]

More general feedback:


The code example in Appendix D uses #include <OpenCL/opencl.h> but the official Khronos header file is named cl.h. It would be nice if everyone could settle on the same include statement so we don't get stuck with a platform #ifdef in every application like we did with OpenGL. Since the specification describes a C api, it's not unreasonable that it actually specify this.
[/*:m:168j75vi]
The description of the glGet*() functions in section 4 are vague on what it means to return a 'char[]' value. I assume this is a C string, and should be copied into the location pointed to by param_value, but it would be nice if this could be clarified. Copy, or treat param_value as a *char[]? Should the string be null terminated? Are they optionally null terminated like in clCreateProgramWithSource(), and if so can the size include the null? (clGetProgramInfo's CL_PROGRAM_SOURCE key specifies a null-terminated string with the size including the null. Kernel function names are also specified as null-terminated in table 5.13.)
[/*:m:168j75vi]
The description of the return values for clCreateContextFromType() should be clarified. Since device_type is a bitfield, there are three options if a device is not available or not found: no devices could be used in the context, some could be used, or all could be used. clCreateContext() is fairly clear: it is an error if "any" requested device could not be used to create the context, so the some case is an error. clCreateContextFromType() would seem to use the opposite: it is only a failure if "no" devices matches, and the some case is successful.
[/*:m:168j75vi]
clRetainContext() and clReleaseContext() return "CL_INVALID_CONTEXT if context is not a valid OpenCL context." Does this mean it must always be safe to pass a bogus context?
[/*:m:168j75vi]
clCreateCommandQueue() says the device parameter may be in the list of devices specified when creating the corresponding context "or have the same device type as device type specified when context is created using clCreateContextFromType." I also really don't like this, it breaks the context as a container for device record keeping. The device_id should be considered invalid if it's not in the context, regardless of whether the type happens to match one that is.
[/*:m:168j75vi]
clGetProgramInfo() when answering the CL_PROGRAM_SOURCE name says the original source lines are concatenated and null terminated. I suggest it be clarified that the concatenation strips any terminal nulls in the original source submission.[/*:m:168j75vi]

cwright
01-13-2009, 03:58 PM
// cl_mem_flags - bitfield
#define CL_MEM_READ_WRITE (1 << 0)
#define CL_MEM_WRITE_ONLY (1 << 1)
#define CL_MEM_READ_ONLY (1 << 2)
#define CL_MEM_USE_HOST_PTR (1 << 3)
#define CL_MEM_ALLOC_HOST_PTR (1 << 4)
#define CL_MEM_COPY_HOST_PTR (1 << 5)

This looks suspicious: Why are there 3 bits to determine if a memory object is Readable and Writable? Why not just have 2 bits, a Read and a Write, and appropriate definitions to include both for Read/Write?

cwright
01-13-2009, 08:35 PM
cl_int clSetCommandQueueProperty (cl_command_queue command_queue,
cl_command_queue_properties properties,
cl_bool enable,
cl_command_queue_properties *old_properties)
...
enable determines whether the values specified by properties are enabled (if enable is CL_TRUE)
or disabled (if enable is CL_FALSE) for the command-queue . The property values are described
in table 5.1.

Since properties (cl_command_queue_properties) is a bitfield, why is there an enable input? Isn't a bit's presence in a bitfield an implied enable (and absence an implied disabled)?

rillian
01-14-2009, 10:08 PM
cwrite, I understood clSetCommandQueueProperty() as setting or clearing (depending on enable) the command queue property bits which have a bit set in the properties argument.

i.e.

if enable == true:
new_properties = old_properties | properties
else:
new_properties = old_properties & ~properties

That is it's a multi-bit mask operation. Not flipping a single bit at a time, nor setting the whole property vector at once.

cwright
01-16-2009, 01:38 PM
rillian, I think you're right -- it was weird to me at first, but after implementing it, it made a lot more sense :) Thanks for jotting down the explanation.

avk
02-11-2009, 11:46 PM
I would like to suggest to add the "Revision History."

Dan Bartlett
03-16-2009, 02:52 PM
Not really something that can be changed now, but the naming scheme doesn't fit too well with case-insensitive languages, such as Delphi.

for example, you have:

type
cl_device_type = cl_bitfield;
cl_device_local_mem_type = cl_uint
cl_device_local_mem_type = cl_uint
cl_mem_flags = cl_bitfield;

and then the same names re-used (but uppercase):

const
// cl_device_info
CL_DEVICE_TYPE = $1000;
CL_DEVICE_LOCAL_MEM_TYPE = $1022;

// cl_context_info
CL_CONTEXT_PROPERTIES = $1083;

// cl_mem_info
CL_MEM_FLAGS = $1101;

these are treated as the same identifiers, so have to be renamed when used with Delphi.

However, the usual naming convention for types in Delphi is a leading T on the name, so this will "solve" the problem, it just makes it a bit harder converting between C & Delphi.

Tcl_device_type = Tcl_bitfield;
Tcl_device_local_mem_type = Tcl_uint
Tcl_device_local_mem_type = Tcl_uint
Tcl_mem_flags = Tcl_bitfield;