Anastasia added inline comments.

================
Comment at: clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl:141
+kernel void extended_subgroup(global uint4 *out) {
+  out[0] = get_sub_group_eq_mask();
+#if __OPENCL_C_VERSION__ < CL_VERSION_2_0 && !defined(__OPENCL_CPP_VERSION__)
----------------
svenvh wrote:
> Anastasia wrote:
> > svenvh wrote:
> > > Anastasia wrote:
> > > > I appreciate we haven't addressed the standard header testing 
> > > > holistically yet and this functionality was added in `opencl-c.h` 
> > > > without testing the functions, but would it be better to test each 
> > > > function at least here?
> > > > 
> > > > Even though the final goal should be testing all overloads, this is 
> > > > outside of the scope of this work that is closing the gap between 
> > > > `opencl-c.h` and this experimental function declaration mechanism.
> > > > but would it be better to test each function at least here?
> > > 
> > > I think that defeats the purpose of this test.  This test is meant to 
> > > test the basic functionality of `-fdeclare-opencl-builtins`.  It is not a 
> > > completeness test.
> > > 
> > > A completeness test shouldn't be tied to `-fdeclare-opencl-builtins` 
> > > alone; such a test should also be run against `opencl-c.h`, so this would 
> > > be the wrong test to turn into a completeness test in my opinion.
> > Ok, this test does test functionality selectively but I wouldn't be able to 
> > tell how one selects what to test. Perhaps a comment would help for the 
> > future development...
> There are no formal selection criteria here I think.  The test aims to cover 
> the different functional aspects of `-fdeclare-opencl-builtins`.  The new 
> functional aspect for `-fdeclare-opencl-builtins` in this patch is an 
> extension that is not in `OpenCLExtensions.def`, so it makes sense to add 
> such a case to the test.
> 
> I will add a comment to the top of the test file.
Makes sense! Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95523

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to