OpenCL-specific image2d_t, image3d_t and sampler_t *revised*

I think we both have a lot of patches out here, so I guess we should try to get these resolved soon. Sorry for my delay in the matter.

General comments on the patch:
- Comments are needed on the OpenCLOtherType class, and member funcs. Comments needed on all the special OpenCL checks within the various parts.End comments with proper punctuation. I guess we could also get into an argument if its initialise or initialize? :wink:

- Why are OpenCL types never serialized? This would need to be implemented before it could be checked in or otherwise things like PCH wont work.

- I'm not an expert in mangling, but wouldn't it be better to use something other than a number?

- In ConvertNewType, shouldn't it assert if the OpenCL other type is not recognized?

I'm not totally against combining these types into one class versus my implementation (which only addressed samplers).
Thanks,
Tanya

I think we both have a lot of patches out here, so I guess we should try to get these resolved soon. Sorry for my delay in the matter.

General comments on the patch:
- Comments are needed on the OpenCLOtherType class, and member funcs. Comments needed on all the special OpenCL checks within the various parts.End comments with proper punctuation. I guess we could also get into an argument if its initialise or initialize? :wink:

- Why are OpenCL types never serialized? This would need to be implemented before it could be checked in or otherwise things like PCH wont work.

- I'm not an expert in mangling, but wouldn't it be better to use something other than a number?

- In ConvertNewType, shouldn't it assert if the OpenCL other type is not recognized?

I'm not totally against combining these types into one class versus my implementation (which only addressed samplers).

However, I will say that these OpenCL types are all different, so I'd probably ask Doug what he thinks is the best to do from a Clang coding standard viewpoint (break them up into separate type classes or have them in one).

-Tanya

I think we both have a lot of patches out here, so I guess we should try to get these resolved soon. Sorry for my delay in the matter.

General comments on the patch:
- Comments are needed on the OpenCLOtherType class, and member funcs. Comments needed on all the special OpenCL checks within the various parts.End comments with proper punctuation. I guess we could also get into an argument if its initialise or initialize? :wink:

- Why are OpenCL types never serialized? This would need to be implemented before it could be checked in or otherwise things like PCH wont work.

- I'm not an expert in mangling, but wouldn't it be better to use something other than a number?

- In ConvertNewType, shouldn't it assert if the OpenCL other type is not recognized?

I'm not totally against combining these types into one class versus my implementation (which only addressed samplers).

However, I will say that these OpenCL types are all different, so I'd probably ask Doug what he thinks is the best to do from a Clang coding standard viewpoint (break them up into separate type classes or have them in one).

I don't like the idea of an OpenCLOtherType class, because it lumps together unrelated types. The type classes should be meaningful, e.g., OpenCLImageType and OpenCLSamplerType.

  - Doug

Hi Tanya and Doug,

Thank you for your comments. We will address them and resubmit... after the
Royal Wedding :slight_smile:

I guess we could also get into an argument if its initialise or

initialize? :wink:
Hmm, I'm not totally against 'initialize' :)).

Anton.

Please review a revised patch for the OpenCL-specific types (now including
event_t). Apologies for the long delay.

Thanks,
Anton.

0000001-opencl-specific-types.patch (52.7 KB)

Anton,

Thanks for breaking these types out into their own class.

Some comments:
- Name mangling: I was under the assumption that vendor extensions had to be in the form of u<string>. Perhaps thats not correct?

- Serialization: I believe this will be required for people who want to use PCH, so I think these functions should be implemented.

- Sampler: I notice that you are not emitting an initializer and only emitting metadata. Why is this? This is going to break any backend that doesn't handle the metadata the same way. I'm not sure why metadata is even necessary.

- ImpCastExprToType: sampler special case. How does codegen take care of this?

- Nit pick: End comments with proper punctuation (missing a bunch of periods).

Thanks,
Tanya