[PATCH] D106343: [OpenCL] Support cl_ext_float_atomics

[Sven van Haastregt via Phabricator via cfe-commits]

svenvh added a comment.

Thanks for the update!  I have a few points to improve the patch.



================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:1107
+// The functionality added by cl_ext_float_atomics extension
+let MinVersion = CL30 in {
+  foreach TypePair =
----------------
Do we really need to guard these additions behind OpenCL 3.0?  The spec mentions
> The functionality added by this extension uses the OpenCL C 2.0 atomic syntax and hence requires OpenCL 2.0 or newer.
(same applies to the opencl-h.c changes of course)


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:1112
+      foreach AddressSpaceiKind = [GenericAS, GlobalAS, LocalAS] in {
+      def:
+        Builtin<"atomic_fetch_" #ModOp, [
----------------
The feature macros seem to be missing.  See `FuncExtOpenCLCPipes` for an example how to do that.


================
Comment at: clang/lib/Sema/OpenCLBuiltins.td:1132-1135
+  foreach TypePair =
+      [[AtomicFloat, Float, Float], [AtomicDouble, Double, Double]] in {
+    foreach ModOp = ["max", "min"] in {
+      foreach AddressSpaceiKind = [GenericAS, GlobalAS, LocalAS] in {
----------------
This can be merged into the preceeding `foreach` parts I think?


================
Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:139
+
+  resf1 = atomic_fetch_min(a_float, f1);
+  resf1 = atomic_fetch_max(a_float, f1);
----------------
As mentioned in the comment on lines 13-17, this test is not meant to be exhaustive.  So you don't have to test every overload, checking one or two builtins should suffice.


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

https://reviews.llvm.org/D106343