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