Anastasia added a comment. In D89372#2335627 <https://reviews.llvm.org/D89372#2335627>, @jvesely wrote:
> In D89372#2335027 <https://reviews.llvm.org/D89372#2335027>, @Anastasia wrote: > >> In D89372#2334728 <https://reviews.llvm.org/D89372#2334728>, @jvesely wrote: >> >>> In D89372#2334225 <https://reviews.llvm.org/D89372#2334225>, @Anastasia >>> wrote: >>> >>>>> >>>> >>>> Ok, so the pragma is supposed to control the pointer dereferencing which >>>> seems like a valid case but however, it is not implemented now. Do we know >>>> of any immediate plans from anyone to implement this? If not I would >>>> prefer to remove the pragma for now? If someone decided to implement this >>>> functionality later fully it is absolutely fine. Pragma will be very easy >>>> to add. There is no need for everyone to pay the price for something that >>>> can't be used at the moment. >>> >>> I though the current behaviour of 'you can use #pragma, but the >>> dereferences are always legal' was intentional for backward compatibility. >>> I don't think there are programs that would set it to 'disabled' and expect >>> compilation failure. >> >> The initial state of the pragma is disabled, so if disabling the extension >> isn't supposed to do anything then I don't know why would anyone ever enable >> it? >> >>> My concern is that legacy apps would set '#pragma enabled' before using >>> char/short. such behavior would produce warning/error if with this change >>> applied. >> >> Correct, it will compile with a warning but not fail to compile. I am >> willing to introduce a -W option (if not present already ) to suppress that >> warning if it helps with the clean up and backward compatibility. It might >> also open up opportunities for a wider clean up - removing pragma in >> extensions that currently require pragma for no good reason. I have written >> more details on this in >> https://github.com/KhronosGroup/OpenCL-Docs/pull/355#issuecomment-662679499 > > Adding a new option cannot be a solution as legacy applications were written > before its introduction. > CL1.x applications need to continue to work without changes, anything else > breaks backward compatibility. Yes, we don't break backward compatibility of the specified behavior. This is never the intent. >>>>> The same arguments also apply to `cles_khr_in64`. It's illegal to use >>>>> int64 types in embedded profile unless you first enable the extensions. >>>>> Rather than removing it, `cles_khr_2d_image_array_writes` should be added >>>>> to the extension list. >>>> >>>> I don't think clang currently support embedded profile. Adding extension >>>> into the OpenCLOptions is pointless if it's not used. Do you plan to add >>>> any functionality for it? Defining macros can be done easily outside of >>>> clang source code or if it is preferable to do inside there is >>>> `MacroBuilder::defineMacro` available in the target hooks. Currently for >>>> every extension added into `OpenCLExtensions.def` there is a macro, a >>>> pragma and a field in `OpenCLOptions` added. This is often more than what >>>> is necessary. Plus Khronos has very many extensions if we assume that all >>>> of them are added in clang it will become scalability and maintanance >>>> nightmare. Most of the extensions only add functions so if we provide a >>>> way to add macros for those outside of clang code base it will keep the >>>> common code clean and vendors can be more flexible in adding the >>>> extensions without the need to modify upstream code if they need to. I see >>>> it as an opportunity to improve common code and out of tree >>>> implementations too. It just need a little bit of restructuring. >>> >>> My understanding is that both the macro and working #pragma directive is >>> necessary. >> >> I don't believe this is the correct interpretation. If you look at the >> extension spec s9.1 it says: >> >> `Every extension which affects the OpenCL language semantics, syntax or adds >> built-in functions to the language must create a preprocessor #define that >> matches the extension name string. This #define would be available in the >> language if and only if the extension is supported on a given >> implementation.` > > This is only one part indicating the availability of extension. It's not > describing what's necessary to observe extended behaviour. The specs state > that the initial state of the compiler is as if `#pragma OPENCL EXTENSION all > : disable` was issued. > This means that functions introduced by extensions cannot be present, and the > symbol names are available for application use unless such symbols were > previously reserved. This is not written in the spec. The statement you quote only says that there is implicit `#pragma OPENCL EXTENSION all : disable` and this is all. It says nothing about functions! >> It does not say that the pragma is needed, it only says that macro is >> needed. That perfectly makes sense because the macro allows to check that >> the extension is present to implement certain functionality conditionally. > > This cited line only talks about extensions availability, not extension use. > One example of newly introduced functions is in cl_khr_in64_base_atomics. The > specs say: > > The optional extensions cl_khr_int64_base_atomics and > cl_khr_int64_extended_atomics > implement atomic operations on 64-bit signed and unsigned integers to > locations in __global > and __local memory. . > An application that wants to use any of these extensions will **need** to > include the #pragma > OPENCL EXTENSION cl_khr_int64_base_atomics : enable or #pragma > OPENCL EXTENSION cl_khr_int64_extended_atomics : enable directive in > the OpenCL program source > > (emphasis mine) The use of pragma is not voluntary, but necessary for the > functions to become available. No, once again is doesn't say this. It doesn't say anything of what happens. And btw we are diverging from the topic I feel. In this patch we are not covering those extensions. This will be more useful as a genral RFC or an issue in github. >> OpenCL spec however never clarified the use of pragma that technically makes >> sense because the pragmas in C languages are used for altering the standard >> behavior that can't be otherwise achieved using standard parsing i.e. C99 >> section 6.10.1 says about non-STDC pragmas: >> >> `The behavior might cause translation to fail or cause the translator or >> the resulting program to behave in a non-conforming manner. Any >> such pragma that is not recognized by the implementation is ignored.` >> >> So C99 only regulates behavior of STDC pragmas and for those it explicitly >> says how the behavior of the parsed program is altered. >> >> Technically OpenCL pragmas are not covered neither in OpenCL C and not C99 >> and therefore it is unclear what the implementation should do. However, with >> time due to the absence of the clarification mutiple interpretations >> appeared. Sadly some of them ended up in a very suboptimal state both for >> the tooling and the application developers because the pragma started to be >> added for no reason or for controlling redundant diagnostics of use of types >> or functions that extensions were introducing. If you are interested in more >> details I suggest to check this issue: >> https://github.com/KhronosGroup/OpenCL-Docs/issues/82 >> >>> The configuration bit is only needed if clang changes behaviour, which is a >>> separate question. >>> I'd also argue that new third party extensions need an API call to register >>> new extensions in order to get a working pragma mechanism. >> >> Can you explain this please? > > explained above, extensions shouldn't introduce new symbols without being > enabled. applications are free to use any symbol names that are not reserved > by the specs. There is nothing of this in the spec. > If my application decides to implement `swap_atomically` it's perfectly free > to do so and should continue to work even if someone later decides to add > `swap_atomically` as extension function. > Requiring such an application to be updated to check for new_extension macro > breaks backward compatibility. This is how it is and has always been. For that Khronos recommends not to use `cl_`-prefixed identifiers because they are likely to be used by extensions or future standards. >>> Even if an extension only adds access new functions, pragma should control >>> if user functions with conflicting names are legal or not. >>> >>> for example, a program can implement function `new_func`, which gets later >>> added to an extension. since the program doesn't know about the new >>> extension it doesn't use `#pragma new_extension:enable` and continues to >>> work correctly. >> >> This is absolutely not how it works or what the pragma currently does or >> even can do. If you want such a thing to work you need to introduce a >> concept of a namespace into OpenCL C, so every extension is implemented as a >> separate namespace to avoid name clushing. I do acknowledge however that it >> would be extremly useful and convenient but it is not how C parsers work. >> And I imagine it is not trivial to extend this but probably not impossible. >> >> https://godbolt.org/z/d8c5fb >> >> If you look at this example the function from the extension have already >> been added into the list the symbal tables. The pragma doesn't undo or >> remove them or make them invisible. There is no machnism to do this easily >> and the proper solution would be to use namespaces but it is only available >> in C++. > > Namespaces are not necessary. the behaviour is easily implementable using > defines: > `#pragma abcdef: enable` introduces `#define exported_function > __reserved_name_function` > `#pragma abcdef: disable` removes such definitions. > > applications cannot legally use names that begin with two underscores, so > there's no risk of name clash I don't understand your proposal but more importantly this is not the right place to discuss it. You are more than welcome to send it to Clang community and to Khronos for the future standards. Right now we talk about what exists in the specification. > `all identifiers that being with an underscore and a capital letter or > another underscore are always reserved for any use` (C99 7.1.3) > > The following names are reserved for use as keywords in OpenCL C and shall > not be used > otherwise. > * Names reserved as keywords by C99. > > Opencl-1.1 6.1.9 > >>> If the new extension exposes `new_func` unconditionally, the program >>> breaks, because it doesn't check for a macro that didn't exist at the time >>> it was written. >>> more recent programs can use ifdef to either use `new_func` provided by the >>> extension, or implement a custom version. >> >> Yes, this is the intent of defining the extension macros. > > And it misses the point. legacy application cannot check for macros that > didn't exist at the time of writing. > requiring applications to be continually adjusted as new extensions become > available breaks backward compatibility. > >>> I didn't find much about embedded program behavior in full profile >>> implementation in the specs. >>> It only says that "embedded profile is a subset" which imo implies that >>> legal embedded profile programs should work correctly in a full profile >>> implementation. >>> This implies that cles_* pragmas should continue to work even if the >>> behavior is always supported. >> >> If there is no proper specification for this I think adding it to clang >> contradict the development policy http://clang.llvm.org/get_involved.html >> >> A specification: The specification must be sufficient to understand the >> design of the feature as well as interpret the meaning of specific examples. >> The specification should be detailed enough that another compiler vendor >> could implement the feature. > > Both cles_khr_int64 and cles_khr_2d_image_array_writes are described in > opencl 1.1 and 1.2 specs. > Applications that use opencl 1.2 follow those specs, and clang's std=cl1.2 > should follow cl 1.2 specs. > Requiring applications to be continually rewritten as new cl versions become > available breaks backward compatibility. > >>> I'd phrase it differently. extensions that modify the language should be >>> included even if clang decides not to implement given language modification. >> >> Why do we need to add what we don't implement? I don't think this complies >> to the clang development policy either as we don't support incomplete >> implementations. > > The extension is already implemented (char/short dereferences work). it just > needs to be exposed to applications performing correct steps to enable > extended behaviour. > The same reason why `cl_khr_fp64` is still listed in the extensions list even > though it stopped being an extension in cl1.2. `cl_khr_fp64` is properly and fully implemented in Clang. It is not in the category being discussed here. You are conflating very different situations. >>> I think it's perfectly fine to always allow dereferencing of char/short >>> types, as long as correctly written CL1.0 programs that use `#pragma OPENCL >>> cl_khr_byte_addressable_store: enable` don't produce extra errors/warnings. >>> Full profile is a superset of embedded profile so the cles_* behaviours >>> should be available and #pragma should work as well. >>> >>> This would suggest that there should be a list of no-op extensions that are >>> only present for backward/embedded compatibility and don't change behaviour >>> since the extended version of language behaviour is always supported. >> >> What do the extensions do? If they do nothing why do they need to be in >> clang? Once again if anyone needs to define a macro, this can be done >> outside of clang code base. OpenCLExtensions.def is not meant to be used >> for adding the macros as it does a lot more than this. Using >> `OpenCLExtensions.def` for only for adding a macro is a hack to me. > > The presence of these extensions enables correct functionality of procedures > that enable extended behaviour (use of pragmas). Irrespective of whether the > same procedure is required in later versions or not. > Correctly written OCL 1.0 applications are required to use legacy pragmas, > removing them breaks backward compatibility. > > Whether it's implemented as a no-op extension, or just a list of pragma names > that shouldn't issue warning or error is an implementation detail. > >>> note that my concerns are purely wrt. spec wording and intentions. Given >>> that ocl didn't see wider use until later versions, such legacy >>> applications might not exist. >> >> I disagree on that there are more OpenCL 1.x conformant devices than OpenCL >> 2.x. You can see by the adoption list. > > sure. which makes it even more puzzling that you don't want to consider cl1.x > specs. Have I ever written that? Right now we are fixing incorrect behavior in the compiler that is not supported by anything in the spec. >>> feel free to reject them as purely academic, but the point is that changes >>> in this revision break backward compatibility (which is always a >>> maintenance burden). >> >> I think we already agreed earlier to leave certain extensions in for now but >> add a comment to address this later. See: >> >> I am ok to leave the extensions that could hypotetically modify the >> language out of this patch for now. Perhaps we could add a comment >> explaining that they are unused and only left for backwards compatibility. >> In a long term we need to find some ways to remove the legacy that doesn't >> bring any benefit to the community. Maybe I will write another RFC and ask >> vendors to reply in the mainling list if they use those and plan to either >> complete the functionality upstream or remove them. > > This is IMO completely misguided. The policy wrt to applications that follow > older spec should be clearly stated and adhered to, giving enough time for > any affected users to adapt their workloads in case there are changes. > An ill-defined 'benefit of the community' just means that old applications > will get broken unless they actively watch for clang development. at which > point it's easier to update legacy apps. We do and will always provide backwards compatibiliy in accordance with the standard. If developers however implemented the applications based on the behavior that the spec doesn't document there is no guarantee provided for such cases. If developers need such a guarantee they have to submit bugs to implementation or/and ask for the spec clarifications to make sure they only rely on the behavior that is written in the spec explicitly Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89372/new/ https://reviews.llvm.org/D89372 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
