Anastasia added inline comments.
================ Comment at: clang/include/clang/Basic/OpenCLOptions.h:37 + // OpenCL Version + unsigned CLVer = 120; + bool IsOpenCLCPlusPlus = false; ---------------- Anastasia wrote: > Anastasia wrote: > > azabaznov wrote: > > > Anastasia wrote: > > > > Ok, I see why you are adding this field but I am not very happy with > > > > it. However, if you prefer to keep it I suggest to add a comment > > > > otherwise it is mislediang because ideally in Clang we should be using > > > > versions from the LangOpts everywhere. Alternatively we could consider > > > > a helper function to calculate the version although it doesn't > > > > eliminate the need to the comment. > > > > However, if you prefer to keep it I suggest to add a comment otherwise > > > > it is mislediang because ideally in Clang we should be using versions > > > > from the LangOpts everywhere > > > > > > The reason for this is that `LangOptions` is not always available for > > > proper settings. So this just needed internally. Also, we could add > > > `TargetInfo::setOpenCLOptions(const LangOptions &Opts)` where we can set > > > all extensions/features in one step (invoke `::setSupportedOpenCLOpts` > > > and `::setOpenCLExtensionOpts`) to make sure `OpenCLOptions` will get > > > proper OpenCL version. What do you think of that? > > Yes this feels neater! > I feel this just results in more layering violations than we have already > made in the past. The version should really be stored in the `LangOpts` and > we can either have a reference to it here or/and a helper function to compute > the lang version equivalent for the extensions given `LangOpts`. > > Perhaps this relates to the fact that `OpenCLOptions` are completely > duplicated in Sema... maybe they should have just been there and not in the > target settings after all... Or perhaps we are in need for some bigger > restructuring in order to avoid making more mess... FYI, I looked at the duplication of `OpenCLOptions` in `Sema` and `TargetInfo`, and my conclusion is that the reason why it was copied to `Sema` is that `TargetInfo` is expected to be immutable throughout the parsing, which makes sense because parsing source shouldn't modify the target being compiled for. The reason why `OpenCLOptions` need to be modified is to set the `Enable` value when pragma is being parsed. This should have been separated to `Sema` and the rest would have stayed as a part of the `TargetInfo`. In reality, most of pragmas are useless so we don't even need to set anything for them aside from perhaps `fp64`. I am planning to remove most of them but it will take some time. Now there is another obstacle - serialization to/from PCH, that now sets `OpenCLOptions` of the `Sema`. I don't think this should have been done this way because it seems bizarre to me that extensions are being overridden after importing PCH module. The implementation was done for the internal header use case but if any customer PCH is imported the flow would break. I don't have a solution to this one yet, but it is clear that this violates the standard PCH flow where the target configuration should be checked to match exactly the configuration being compiled for and if they don't match the import should fail. There should be no overriding of target settings by the PCH import. This will be trickier to fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89869/new/ https://reviews.llvm.org/D89869 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
