[PATCH] D96860: [OpenCL] Add declarations with enum/typedef args

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added inline comments.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:932
+let MinVersion = CL20 in {
+  def : Builtin<"get_fence", [MemFenceFlags, PointerType<Void, GenericAS>]>;
+  def : Builtin<"get_fence", [MemFenceFlags, PointerType<ConstType<Void>, GenericAS>]>;
----------------
Anastasia wrote:
> svenvh wrote:
> > Anastasia wrote:
> > > Btw, I am guessing you are matching the behavior of `opencl-c.h`, but however it doesn't seem entirely conformant.
> > > 
> > > `cl_mem_fence_flags get_fence(gentype *ptr)`
> > > 
> > > So I guess we should have implemented these as other functions from `Table 22. Built-in Address Space Qualifier Functions` in clang? But since the void pointer is only used as a parameter and not return value I doubt this is customer visible. Should we add a comment or something?
> > > 
> > > 
> > > 
> > Yes, I'm matching  `opencl-c.h` here, as the "gentype argument to indicate any of the built-in data types supported by OpenCL C or a user defined type.  Since this cannot be captured" can't be expressed literally.  I can add the following comment if that makes sense?
> > ```// The OpenCL 3.0 specification defines these with a "gentype" argument indicating any builtin
> > // type or user-defined type, which cannot be represented currently.  Hence we slightly diverge
> > // from this by providing only the following two overloads with a void pointer.
> > ```
> Yes, I think a comment is good enough. We could even create a spec bug because it is literally not possible to implement without a **magic** and just using `void*` is so easy and straight forward.
Added the comment.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:1103
   }
   foreach TypePair = [[AtomicInt, Int, Int], [AtomicUInt, UInt, UInt],
                       [AtomicLong, Long, Long], [AtomicULong, ULong, ULong],
----------------
Anastasia wrote:
> I think this should be renamed now, but we can do as a separate change.
> 
> I would suggest using something like `OverloadTypes` instead...
Agreed that renaming belongs to a separate patch, we can probably bikeshed over the actual name. `TypeTuple` would already be better than `TypePair` for this case.  I find `OverloadTypes` a bit ambiguous (I guess with "Overload" you intend to refer to a particular instance of the overloaded function, but not sure if it's common to use "overload" as a noun).  `ReturnTypeAndArgumentTypes` or `OverloadedFunctionTypes` seem more accurate but a bit unwieldy...


================
Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:185
+#if !defined(NO_HEADER) && (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
+  sub_group_barrier(CLK_GLOBAL_MEM_FENCE, memory_scope_device);
+#endif
----------------
Anastasia wrote:
> svenvh wrote:
> > Anastasia wrote:
> > > Should `work_group_barrier` and fences also be added to the testing?
> > MemFenceFlags are already tested using `barrier()` on line 23, so not strictly needed.  I added this test to verify the combination of MemFenceFlags and extensions.  Do you think we're missing any tablegen functionality coverage that would require adding more builtins to the test (also taking into account the comment on lines 10-13)?
> Taking into account the definition it's not easy to tell. I was trying to apply the strategy of any new group of declarations is to be tested but it might not be the original intention. I still think we do need to come up with some sort of unambiguous strategy but perhaps it is not in the scope of this patch. 
> any new group of declarations is to be tested but it might not be the original intention.

That has not been the convention for this test indeed.  It's a bit hard to give an exact definition when the purpose of the test is to do a quick sweep across the main functional aspects of the `-fdeclare-opencl-builtins` machinery.  As mentioned in various other reviews, a full test for all OpenCL builtins is desirable to have, but outside the scope of this test (which is probably more like a best-effort test).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96860/new/

https://reviews.llvm.org/D96860