This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add diagnostics for references to functions
ClosedPublic

Authored by Anastasia on Jan 26 2021, 6:30 AM.

Details

Summary

References to functions are less permissive than pointers to functions and therefore some use cases could be allowed for example:

  • For local variables
  • For non-extern static and global variables
  • For template parameters that are not used as function parameters or class members
  • In constexpr

However, more language work is needed to analyze the invalid/valid cases. This can be done in the future language versions if there is enough interest in the feature from the application developers. Even if we had a request for allowing some function pointer functionality (PR44788), OpenCL has never adopted the feature as a part of the standard and therefore many applications have been written successfully without it. On the other hand C++ provides other related features - lambdas and function objects that are allowed in OpenCL and can be used to express some sort of the indirect function call logic. For now it seems reasonable to just disallow the references to functions just like we disallow function pointers. This prevents erroneous programs from being compiled silently.

Note that for the advanced users there is the following extension available __cl_clang_function_pointers that can be used if there is knowledge about the application sources or compilation options to make sure the non-conformant functionality is safe (more details are in https://clang.llvm.org/docs/LanguageExtensions.html#opencl-features).

Diff Detail

Event Timeline

Anastasia created this revision.Jan 26 2021, 6:30 AM
Anastasia requested review of this revision.Jan 26 2021, 6:30 AM
Anastasia added inline comments.Jan 26 2021, 8:09 AM
clang/test/SemaOpenCLCXX/references.cl
24

I have created PR48887 for this because we also miss other diagnotics.

29

oops, I thought this was covered in my patch. I will see if there is a quick fix and if not I will create another PR.

Improved diagnostics to cover more cases.

NOTE that this now also contains similar improvements for the pointers to member functions.

Anastasia added inline comments.Jan 26 2021, 11:44 AM
clang/test/SemaOpenCLCXX/references.cl
29

Ok, I have addressed that. I realized that my original patch was not doing what I said in the description as it was still allowing the references to functions in type aliases or template arguments. While I think such functionality might be useful we should first start from the language definition side of it. For now let's just go with the conservative approach and disallow this.

Looks sensible to me.

clang/test/SemaOpenCLCXX/members.cl
17 ↗(On Diff #319369)

I wonder, do we lose coverage by removing these templates?

In other words, is the same code for error detection used for templates and non-template?

clang/test/SemaOpenCLCXX/references.cl
26

This re-uses the same name as above. Could that be an issue?

Anastasia added inline comments.Jan 27 2021, 8:36 AM
clang/test/SemaOpenCLCXX/members.cl
17 ↗(On Diff #319369)

I just think that this pattern has become untestable which means it is also not customer visible i.e. I used these to create a function pointer type in a template via template argument that was a reference to function. But now that we don't have neither pointers nor references to functions I don't think it is possible to write a template that would have any of those two after the substitution? I think the error will occur before - either when we specify the template argument or create a variable of those invalid types to be deduced in the templates.

clang/test/SemaOpenCLCXX/references.cl
26

Not in this case because the type alias has not been created i.e. the parser just restarts over as if the first line with this type alias has not existed.

mantognini accepted this revision.Jan 27 2021, 8:41 AM

Right, thanks for the explanations. They make sense.

This revision is now accepted and ready to land.Jan 27 2021, 8:41 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2021, 7:08 AM