[patch] initial OpenCL C language support
Ken Werner
ken@linux.vnet.ibm.com
Wed Nov 3 13:03:00 GMT 2010
On Tuesday, November 02, 2010 8:23:11 pm Joel Brobecker wrote:
> > > Just to clarify a bit what I was trying to say: Most of the comments
> > > are mostly cosmetic, and I don't think we need to verify that you
> > > followed the comments correctly. If this was the only comments I had,
> > > I would be comfortable with pre-approving the patch, particularly
> > > since Tom already looked at it as well. But before the patch goes in,
> > > I'd like to understand what the reason for the changes in c-exp.y...
> >
> > The reason for looking up the primitive types instead of referring to
> > the builtins is that the builtin types may have a wrong type size. The
> > OpenCL type long for example is expected to have a size of 8 byte
> > while the size of the GDB builtin long is dependant on the current
> > architecture and might be only 4 bytes.
>
> But you have the gdbarch vector when building the OpenCL long, right?
> (see opencl_language_arch_info). For instance, in Ada, we don't know
> the size of type "Integer", so we ask the gdbarch what is the size of
> int.
>
> lai->primitive_type_vector [ada_primitive_type_int]
> = arch_integer_type (gdbarch, gdbarch_int_bit (gdbarch),
> 0, "integer");
>
> That way, we can use the builtin types directly. Would that work in
> your case?
In case of C (and probably Ada as well) the lengths of the builtin types
depend on the architecuter/implementation but for OpenCL C it's kind of the
other way round. I hard coded the lengths because OpenCL specifies fixed sizes
of the builtin types:
http://www.khronos.org/registry/cl/sdk/1.1/docs/man/xhtml/scalarDataTypes.html
http://www.khronos.org/registry/cl/sdk/1.1/docs/man/xhtml/vectorDataTypes.html
Does this sound reasonable?
> > + /* Triple vectors have the size of a quad vector. */
>
> ^^^ missing second
> space
Fixed.
> > +/* Returns non-zero if the array ARR contains duplicates within
> > + the first N elements. */
>
> non-zero should be spelled nonzero. I have known that for quite a while
> because a friend of mine is really good at spelling, but never really
> thought much until I saw this being explicitly mentioned in the GCC
> Coding Conventions. Let's try to fix them one at a time...
Changed.
> > + for (i = offset; i < n; i++)
> > + {
> > + memcpy (value_contents_raw (v) + j++ * elsize,
> > + value_contents (c->val) + c->indices[i] * elsize,
> > + elsize);
> > + }
>
> The curly braces are unnecessary in this case.
Fixed.
> > + for (i = start; i < end; i++)
> > + {
> > + int startoffset = (i == start) ? startrest : 0;
> > + int length = (i == end) ? endrest : elsize;
> > + if (!value_bits_valid (c->val, c->indices[i] * elsize +
> > startoffset, + length))
>
> Missing empty line after variable declarations...
Added.
> > + for (i = 0; i < c->n; i++)
> > + {
> > + if (value_bits_valid (c->val, c->indices[i] * elsize, elsize))
> > + return 1;
> > + }
>
> Unecessary curly braces...
>
> > + for (i = 0; i < n; i++)
> > + {
> > + /* Copy src val contents into the destination value. */
> > + memcpy (value_contents_writeable (ret)
> > + + (i * TYPE_LENGTH (elm_type)),
> > + value_contents (val)
> > + + (indices[i] * TYPE_LENGTH (elm_type)),
> > + TYPE_LENGTH (elm_type));
> > + }
>
> Likewise.
Fixed.
> > +/* Perform a relational operation on two operands. */
> > +static struct value *
> > +opencl_relop (struct expression *exp, struct value *arg1, struct value
> > *arg2, + enum exp_opcode op)
>
> Missing empty line after the function description.
>
> > + if (!t1_is_vec && !t2_is_vec)
> > + {
> > + int tmp = scalar_relop (arg1, arg2, op);
> > + struct type *type =
> > + language_bool_type (exp->language_defn, exp->gdbarch);
> > + val = value_from_longest (type, tmp);
> > + }
>
> Missing empty line after variable declarations...
Both Added.
> > +# Increase timeout
> > +set timeout 60
> > +verbose "Timeout set to $timeout seconds" 2
>
> Have we determine why it is necessary to increase the timeout to 60?
I encountered timeouts when running the OpenCL testsuite on a system that is
busy with other things as well. As the default value of ten seconds is usually
sufficient I removed the increase and cleaned up some other places as well.
Thanks for bringing that up again.
An updated patch is attached.
Thanks
Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opencl-lang.patch
Type: text/x-patch
Size: 191587 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20101103/4aa779e1/attachment.bin>
More information about the Gdb-patches
mailing list