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