This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Clang diagnostics allow reporting C++ for OpenCL version.
ClosedPublic

Authored by Topotuna on Aug 6 2021, 7:48 AM.

Details

Summary

Some Clang diagnostics could only report OpenCL C version. Because
C++ for OpenCL can be used as an alternative to OpenCL C, the text
for diagnostics should reflect that.

Output text modified for these diagnostics:
warn_option_invalid_ocl_version
err_attribute_requires_opencl_version
warn_opencl_attr_deprecated_ignored
ext_opencl_ext_vector_type_rgba_selector

Diff Detail

Event Timeline

Topotuna created this revision.Aug 6 2021, 7:48 AM
Topotuna requested review of this revision.Aug 6 2021, 7:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 7:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Topotuna updated this revision to Diff 364796.Aug 6 2021, 7:57 AM

Diagnostic argument index updated (from %1 to %2)

Anastasia added inline comments.Aug 6 2021, 9:41 AM
clang/include/clang/Basic/DiagnosticFrontendKinds.td
248

Since we have a number of diagnostics now that print the current lang version using this pattern do you think we could instead add a helper function that prints the full current OpenCL version spelling. We could add something similar to LangOptions::getOpenCLVersionTuple() so let's say LangOptions::getOpenCLVersionString() that could use a tuple-helper internally. Then we could change such diagnostics to something like:

%0 does not support the option '%1'

This could help us simplifying the source code and make sure the diagnostic wording is always consistent.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2956

Let's simplify this to something like:

attribute %0 is supported in the OpenCL version %1%select{| onwards}3
10150

How about changing to:

vector component name '%0' is a feature from OpenCL version 3.0 onwards

or otherwise, we would need to enumerate all versions where it is supported which is not scalable.

clang/lib/Sema/SemaDeclAttr.cpp
7377

I feel that this condition has assumed OpenCL 2.0 which was not correct. We need to render the current version correctly through the LangOptions::getOpenCLVersionTuple() helper.

Topotuna updated this revision to Diff 365157.Aug 9 2021, 4:54 AM

Diagnostics reworded. Helper function getOpenCLVersionString() added. Tests updated

Topotuna marked 4 inline comments as done.Aug 9 2021, 4:55 AM
Anastasia accepted this revision.Aug 9 2021, 6:00 AM

LGTM! Thanks!

Btw I think we could also use this helper in another diagnostic (err_opencl_unknown_type_specifier) in include/clang/Basic/DiagnosticCommonKinds.td. But it can be updated separately though.

This revision is now accepted and ready to land.Aug 9 2021, 6:00 AM
Topotuna updated this revision to Diff 365179.Aug 9 2021, 7:33 AM

Diagnostic err_opencl_unknown_type_specifier addressed. Deprecation conditions changed in nosvm attribute test.

Great! Thanks!

The suggested minor test clean-up can be addressed in the final commit!

clang/test/SemaOpenCL/nosvm.cl
10

Here we could check for predefined macros __OPENCL_CPP_VERSION instead

This revision was landed with ongoing or failed builds.Aug 13 2021, 5:57 AM
This revision was automatically updated to reflect the committed changes.