azabaznov 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) ---------------- Anastasia wrote: > 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? I mean that you could modify `OpenCLOptions::isWithPragma` that it will check if extension/feature was introduced in OpenCL C 3.0. If that's the case then no pragma needed. This new field `pragma` looks redundant as it is set to true only for OpenCL C 3.0 features. However, it may be more cosmetic concern... ================ Comment at: clang/lib/Parse/ParsePragma.cpp:785 + // Therefore, it should never be added by default. + Opt.acceptsPragma(Name); } ---------------- Anastasia wrote: > 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... Oh, I think we can ignore this as double underscored identifiers are reserved. It's not expected that users will declare new features. 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
