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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
