[PATCH] D37231: Add half load and store builtins

[Jan Vesely via Phabricator via cfe-commits]

jvesely added inline comments.


================
Comment at: include/clang/Basic/Builtins.def:1427
+// OpenCL half load/store builtin
+BUILTIN(__builtin_store_half, "vdh*", "n")
+BUILTIN(__builtin_store_halff, "vfh*", "n")
----------------
Anastasia wrote:
> jvesely wrote:
> > Anastasia wrote:
> > > I think this should be a language builtin (see above) but perhaps we might need to extend the language version here. Because I believe we only have OpenCL v2.0 currently.
> > > 
> > > Also this should only be available if `cl_khr_fp16` is supported and enabled? I think we are doing similar with some subgroups functions (e.g. `get_kernel_sub_group_count_for_ndrange`) that are only supported by `cl_khr_subgroup` but those have custom diagnostic though. May be we could leave this check out since `half` is not available if `cl_khr_fp16` is not enabled anyways.
> > This is specifically meant to be used when `cl_khr_fp16` is **not** available.
> > CLC allows using half as storage format and  half pointers without the extension,
> > vstore_half/vload_half are used to load/store half values. (CL1.2 CH 6.1.1.1)
> > 
> > These builtins are not necessary if `cl_khr_fp16` is available (we can use regular loads/stores).
> > 
> > I'll take stab at making these CLC only, but similarly to device specific builtins it looked useful beyond that, since these builtins provide access to half type storage.
> Strange. This is not how I would interpret from the extension spec though: https://www.khronos.org/registry/OpenCL/sdk/1.2/docs/man/xhtml/cl_khr_fp16.html
> 
> But I think for this change is probably fine indeed because this doesn't affect half type itself.
I'm not sure I see the conflict here. `cl_khr_fp16` adds support for `half` scalar and `halfn` vector types.
without the extension the specs say (`OCL 1.2 Ch. 6.1.1.1`):
> The half data type can only be used to declare a pointer to a buffer that contains half values.
`vload_half` and `vstore_half` used to access those buffers without needing `half` type (or the `cl_khr_fp16` extension).

> But I think for this change is probably fine indeed because this doesn't affect half type itself.
exactly. this is needed outside of `cl_khr_fp16`, or the `half` type




================
Comment at: test/CodeGenOpenCL/no-half.cl:19
+	__builtin_store_half(foo, bar);
+// CHECK: [[HALF_VAL:%.*]] = fptrunc double %foo to half
+// CHECK: store half [[HALF_VAL]], half addrspace({{.}})* %bar, align 2
----------------
Anastasia wrote:
> Would it make sense to add a check for `load` similarly to `store` in the test_load_float/test_load_double tests?
there is no load. `fptrunc double %foo to half` uses the function parameter directly


================
Comment at: test/CodeGenOpenCL/no-half.cl:27
+	foo[0] = __builtin_load_halff(bar);
+// CHECK: [[HALF_VAL:%.*]] = load
+// CHECK: [[FULL_VAL:%.*]] = fpext half [[HALF_VAL]] to float
----------------
Anastasia wrote:
> Minor thing: any reason you are not checking the load fully?
just my laziness, I've added full check.


Repository:
  rL LLVM

https://reviews.llvm.org/D37231