Anastasia 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>]>;
----------------
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.
================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:1103
}
foreach TypePair = [[AtomicInt, Int, Int], [AtomicUInt, UInt, UInt],
[AtomicLong, Long, Long], [AtomicULong, ULong, ULong],
----------------
I think this should be renamed now, but we can do as a separate change.
I would suggest using something like `OverloadTypes` instead...
================
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
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96860/new/
https://reviews.llvm.org/D96860
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
