Differential D137652
Remove mandatory define of optional features macros for OpenCL C 3.0 Authored by FMarno on Nov 8 2022, 9:14 AM.
Details A number of feature macros were being defined for OpenCL C 3.0 even though they are optional. I have removed these defines and updated the related tests. This should help users who are creating compilers for more limited environments. This contribution is being made by Codeplay on behalf of Samsung.
Diff Detail
Event Timeline
Comment Actions Are these features affecting the frontend functionality in any way? If not we should implement those in the headers and if headers are not flexible enough to condition out the addition of the new macros we should extend them (potentially by extending behaviour of -cl-ext or introducing undef macros, see https://github.com/llvm/llvm-project/issues/55674). Please review the guidelines here: https://clang.llvm.org/docs/OpenCLSupport.html#opencl-extensions-and-features. The most relevant part here is:
Overall, the idea is to avoiding modifying frontend changes if we can implement features via the headers instead. This is needed to address the scalability problems.
Comment Actions @Anastasia I feel like I am following the guidance you quoted here. The defines I've deleted in opencl-c-base.h are blocking the possibility of -cl-ext working and would also get in the way of undefine preprocessor symbols working. Comment Actions Let me clarify this topic a bit better - the guidelines are about whether a feature/extension needs to be added into the frontend based on whether the frontend needs to know about the feature and do something different e.g. for example it can parse the code differently and etc. However you are adding the features just in order for them to appear in the predefined macros. The approach you choose is a workaround because ideally it should only be used when the fronted needs to query the feature settings to do something useful based on those but you don't ever need them in the frontend as far as I understand as you only need to add the macros. Since clang includes default header files it is very straightforward to add the macros in the headers by simply defining them there. No frontend changes needed for that. Now the only problem is that the headers don't provide defining the macros conditionally yet because nobody has added this functionality yet. One idea that we discussed in the issue I quoted is to extend -cl-ext such that it would be able to define or undefine those macros, or alternatively feature/extensions macro defines in the headers could be guarded by the corresponding __undef_<feature/extension name> macros that can be added from the command line during source compilation by either user or a compiler driver. It is also possible to use a hybrid approach i.e. -cl-ext would add __undef_<feature/extension name> macros that guard the feature/extension macros in the header. This mechanism of adding header only feature/extension macros is not fully working yet as it is currently only sets macros per target. I appreciate that a more fine grained control is desirable in some situation as in your case. However this doesn't mean that we should continue adding more thing into the frontend where it is not needed. We should work toward more scalable solution instead that provides such control in the internal headers. Does this make the direction clear? I would recommend to check the discussions in the issue (#55674) and linked review as it provides more background. @svenvh I remember that we have also discussed the addition of a vendor specific header where such feature/extension macro definition can be added to avoid the macro pollution but I feel this is somewhat orthogonal i.e. the fine grained control of macro defines is still needed? Comment Actions
Unfortunately I don't remember the details of that discussion, but I agree that it's worth looking into a solution for issue #55674, using e.g. __undef macros as you described above. Comment Actions I assume we could start from something simple i.e. without the need to amend -cl-ext. So let's say we could add the following guards in the header: #ifndef __undef_feature1 #define feature1 1 #endif Then we can pass -D__undef_feature1 instead of -cl-std=-feature1 and -Dfeature1=1 instead of -cl-std=+feature1. Then later if needed we could extend -cl-ext to set those __undef_<feature name> instead. We could also add a macro that corresponds to -cl-ext=-all and etc. Comment Actions Thank you for your comments. Unfortunately I won't be able to implement these changes because of time constraints in the current project, sorry if I've wasted your time. | ||||||||||||
I am wondering why those features weren't added together with the other OpenCL 3.0 features; there wasn't any discussion around that in D95776. Perhaps it's because these don't affect the compiler behaviour directly? (but then neither does e.g. __opencl_c_atomic_order_acq_rel) Wondering if @Anastasia has any insights here.