Anastasia added inline comments.
================ Comment at: clang/include/clang/Basic/OpenCLExtensions.def:71 +OPENCL_EXTENSION(cl_khr_int64_extended_atomics, true, 100) +OPENCL_COREFEATURE(cl_khr_3d_image_writes, true, 100, OCL_C_20) ---------------- azabaznov wrote: > Anastasia wrote: > > azabaznov wrote: > > > I think core and optional core features do not require pragma too. So for > > > example 3d image writes or fp64 do not require pragma in OpenCL C 2.0. > > > Isn't that so? > > Well to be honest nothing seems to need pragma but we have to accept it for > > the backward compatibility. :) > > > > In case of the core features if pragma would have been useful we would have > > to still accept it for the backward compatibility even if the feature > > became core. > I'm just wondering if this new field needed in the file to maintain backward > compatibility. Maybe we can highlight OpenCL C 3.0 features with some other > way? Is it seems that check for name starting with "__opencl_c" is a bad idea? Not sure I understand you here but I don't think that we should add extension pragmas any longer at all even for the new extensions. FYI I am planning to add guidelines for that in https://reviews.llvm.org/D97072. Maybe it helps to clarify the idea? ================ Comment at: clang/lib/Parse/ParsePragma.cpp:785 + // Therefore, it should never be added by default. + Opt.acceptsPragma(Name); } ---------------- azabaznov wrote: > Anastasia wrote: > > svenvh wrote: > > > I fail to understand why this is needed, so perhaps this needs a bit more > > > explanation. Extensions that should continue to support pragmas already > > > have their `WithPragma` field set to `true` via `OpenCLExtensions.def`. > > > Why do we need to dynamically modify the field? > > It is a bit twisty here to be honest. Because we have also introduced the > > pragma `begin` and `end` that would add pragma `enable`/`disable` by > > default. So any extension added dynamically using `begin`/`end` would have > > to accept the pragma `enable`/`disable`. > > > > https://clang.llvm.org/docs/UsersManual.html#opencl-extensions > > > > But in the subsequent patches, I hope to remove this because I just don't > > see where it is useful but it is very confusing. > Is it ok not to track this situation here: > > ``` > #pragma OPENCL EXTENSION __opencl_c_feature : begin > #pragma OPENCL EXTENSION __opencl_c_feature: end > ``` > > This is some of a corner case, but still... I see. I am not sure what should happen here - I guess we should give an error? Although for earlier versions than OpenCL 3.0 this should probably be accepted? Perhaps we can create a PR for this for now... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97052/new/ https://reviews.llvm.org/D97052 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
