This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL][Docs] Add guidelines for adding new extensions and features
ClosedPublic

Authored by Anastasia on Feb 19 2021, 11:36 AM.

Details

Summary

Add documentation that explains how to extend clang with new extensions/features.

The guidelines also detail clang's position about the extension pragmas for the new functionality and it is expected the following change to be added first: https://reviews.llvm.org/D97052.

For further clarification around the extension pragma - the most common interpretation of the pragma in clang right now is the diagnostic of the use of types or functions from the extensions:

def err_opencl_requires_extension : Error<
  "use of %select{type|declaration}0 %1 requires %2 extension to be enabled">;

This forces the developers to use the pragma, but it is not very clear why and it seems to violate the following statement in the spec:

disable - Behave (including issuing errors and warnings) as if the extension extension_name is not part of the language definition.

The diagnostic given indicates that the disabled extension is indeed part of the language.

However, I must admit it doesn't seem that implementing dynamic loading/unloading of the extension functionality implied by the spec would be possible in clang. It is not clear why such flexibility would be needed either - most of the language provide one way of importing/including functionality.

Additionally, many extensions add functionality via the implicitly included headers and for using those no pragmas is required as no diagnostic is given if the pragma is not used.

Based on the above the conclusion is that the functionality provided by clang is neither conformant nor consistent for the user.

Diff Detail

Event Timeline

Anastasia created this revision.Feb 19 2021, 11:36 AM

Some minor typos and requests for clarifications, looks like reasonable guidelines other than that.

clang/docs/OpenCLSupport.rst
175

Probably easier to keep this section singular (i.e. talk about a adding a single extension), instead of switching between extension and extensions all the time.

176

associated

177

Again, keep it singular.

179
195
196

This might need some rephrasing to take TableGen BIFs into account. I would consider clang/lib/Sema/OpenCLBuiltins.td an integral part of the clang source code.

Maybe rephrase "clang source code modifications" into "clang parser source code modifications"?

201

Should this mention clang/lib/Sema/OpenCLBuiltins.td too?

212
216
219
221

Remove "acceptable behavior of".

How do you define "useful functionality"?

226
Anastasia updated this revision to Diff 326055.Feb 24 2021, 5:06 AM

Added suggestions from Sven.

Anastasia added inline comments.Feb 24 2021, 5:15 AM
clang/docs/OpenCLSupport.rst
196

I am trying to avoid using parsing in relation to clang since someone might understand it as clang Parser library functionality. I have added a note about this case explicitly

Note that new functionality in `OpenCLBuiltins.td detailed in :ref:<opencl_builtins>` belong to this category even if it is part of the clang source code.

Although we do refer to header functionality in the next paragraph anyway where this is referenced but indirectly. I hope it provides enough clarifications for now.

201

I think here I don't want to go to too many details of how headers are implemented. I refer to this: https://clang.llvm.org/docs/UsersManual.html#opencl-header that also refers to OpenCLSupport page where we explain details on Tablegen header. However, the header topic does require another round of clarifications and I do plan to improve it further iin the near future.

221

I think this is one of those situations that is hard to define unambiguously, so I am open to suggestions. However, I do want to prevent changes that are reinventing the wheel (i.e. C/C++ already had another way to do the same thing) or functionality that doesn't add any value (i.e. requiring pragma enable to use already added types or functions). This has happened in the past multiple times so I think it is good to be specific that when new functionality is added it should have a reason for it.

I think the other statement above:

New functionality is accepted as soon as the documentation is detailed to the level sufficient to be implemented.

is similar. It is not very easy to tell what is the detailed level of documentation. FYI it generally aligns with clang overall policy:

https://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.

I am just emphasizing this item here.

Regarding 'usefulness' I would even suggest adding it as a general guideline for clang but I think this is not very common. So for now I want to make sure we have it in our guidelines at least since we have encountered this.

Some minor comments.

clang/docs/OpenCLSupport.rst
197
219

"of their behavior", I think.

223
svenvh added inline comments.Feb 24 2021, 9:38 AM
clang/docs/OpenCLSupport.rst
196

This makes it very confusing for the reader, because you're referring to "this category" without defining what a category is.

How about removing that note, and appending to the line above instead:

If an extension adds functionality that does not modify standard language parsing it should not require modifying anything other than header files and `OpenCLBuiltins.td as detailed in :ref:<opencl_builtins>`

201

Fair enough; with the addition I'm suggesting above my concern would be addressed anyway.

219

Yes!

221

I see, though I'm a bit hesitant to accept the word "useful" without any further explanation. Would adding the following make sense?

... should provide useful functionality that cannot be achieved by other means to the user.

Anastasia updated this revision to Diff 327199.Mar 1 2021, 11:10 AM
  • Added suggestions from Sven and Marco.
Anastasia marked 13 inline comments as done.Mar 1 2021, 11:11 AM
Anastasia added inline comments.
clang/docs/OpenCLSupport.rst
221

I think this wording doesn't cover the case with pragmas though... where the behavior was not possible to achieve without them but it was not clear why it was needed to achieve what was achieved with the pragmas.

I find useful pretty clear here even though it is not precisely defined. I would not be able to define it precisely at this point because I don't even know all the possible ways the pragmas can be misused. I think we might have to accept the fact that human language is not as precise as a programming language. As a matter of fact, we are not publishing an algorithm but only the guidelines. Although of course, we don't want to misguide, this might not always be avoidable. For the same reasons Clang contribution policy has never defined what is a sufficiently precise documentation format for the new features. This will be left for the community to assess on a case by case basis.

svenvh added a comment.Mar 2 2021, 3:51 AM

LGTM mostly, but would like to hear the opinion of others on the discussion about "useful".

clang/docs/OpenCLSupport.rst
221

this wording doesn't cover the case with pragmas though... where the behavior was not possible to achieve without them but it was not clear why it was needed to achieve what was achieved with the pragmas.

Yes, that's a good point.

I'm still not sure about specifying "useful" without any further clarification then, but if others are happy to accept this then that's fine by me.

Anastasia updated this revision to Diff 329602.Mar 10 2021, 3:55 AM

Added sentence to elaborate the meaning of "useful".

svenvh accepted this revision.Mar 10 2021, 3:59 AM

LGTM with the latest elaboration on "useful". It seems you accidentally added unrelated changes to Types.cpp to this review, so please take care not to commit those.

This revision is now accepted and ready to land.Mar 10 2021, 3:59 AM
Anastasia updated this revision to Diff 329605.Mar 10 2021, 4:00 AM

Fixed patch reupload issue.

This revision was landed with ongoing or failed builds.Mar 11 2021, 6:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 6:29 AM