This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Override supported OpenCL extensions with -cl-ext option
ClosedPublic

Authored by asavonic on Aug 19 2016, 5:20 AM.

Details

Summary

This patch adds a command line option '-cl-ext' to control a set of
supported OpenCL extensions. Option accepts a comma-separated list
of extensions prefixed with '+' or '-'.

It can be used together with a target triple to override support for some
extensions:

// spir target supports all extensions, but we want to disable fp64
clang -cc1 -triple spir-unknown-unknown -cl-ext=-cl_khr_fp64

Special 'all' extension allows to enable or disable all possible
extensions:

// only fp64 will be supported
clang -cc1 -triple spir-unknown-unknown -cl-ext=-all,+cl_khr_fp64

Diff Detail

Repository
rL LLVM

Event Timeline

asavonic updated this revision to Diff 68674.Aug 19 2016, 5:20 AM
asavonic retitled this revision from to [OpenCL] Override supported OpenCL extensions with -cl-ext option.
asavonic updated this object.
asavonic added a reviewer: Anastasia.
asavonic added a subscriber: cfe-commits.
asavonic updated this revision to Diff 68677.Aug 19 2016, 5:43 AM
  • Remove unused test case
Anastasia edited edge metadata.Aug 19 2016, 8:59 AM

What would be the use case to override the supported extensions for the end user?

The change to set the right extensions based on the target compiled for was to avoid mis-compilations. But adding a user flag to control that could lead to the old problem to reoccur.

Anastasia edited reviewers, added: joey; removed: Anastasia.Aug 19 2016, 9:04 AM
Anastasia added a subscriber: Anastasia.

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

For example, when the platform supports fp64, the OpenCL runtime may not have a
fp64 built-ins implementation. In this case it is better to disable fp64 by the
compiler, rather than get a link error.

Actually, the use case is similar to the -target-feature option, which allows to
control supported CPU features, e.g enable or disable some instruction set.

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

Would triple not be suitable in this case, because they include OS/RT information?

bader added a subscriber: bader.

Hi @Anastasia,

Sorry for my late reply.

What would be the use case to override the supported extensions for the end user?

Some extensions may be supported by the platform in general, but not by the
specific version of the OpenCL runtime.

Would triple not be suitable in this case, because they include OS/RT information?

This can be achieved with a custom triple, yes, but it is not a flexible solution.
We need to create a triple for every combination of extensions. If we want to change it, we need to modify the clang source code and recompile it.

I think it is better to have a default set of extensions (from target triple) and allow user to tweak it without creating one more triple.

So instead of:

clang -cc1 -triple spir-unknown-intel-skl-nofp64-nofp16 <...>

we could write:

clang -cc1 -triple spir-unknown-unknown -cl-ext=-cl_khr_fp64,-cl_khr_fp16 <...>

Both have the same result, but the latter one is more flexible and it can be changed without changing the clang code.

yaxunl edited edge metadata.Sep 20 2016, 8:31 AM

I think we need two more tests for concatenating and overriding the option, e.g

-cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64

and

-cl-ext=-cl_khr_fp64,+cl_khr_fp64
include/clang/Basic/OpenCLOptions.h
35 ↗(On Diff #68677)
nm = Enable;
39 ↗(On Diff #68677)

It seems Enable should be a local variable.

59 ↗(On Diff #68677)
nm = Enable;
include/clang/Driver/Options.td
395 ↗(On Diff #68677)

maybe something like

... Enable or disable specific OpenCL extensions separated by comma. Use 'all' for all extensions.
asavonic updated this revision to Diff 72143.Sep 22 2016, 1:28 AM
asavonic edited edge metadata.

Add more test cases and fix minor issues

asavonic marked 3 inline comments as done.Sep 22 2016, 1:40 AM
asavonic added inline comments.
include/clang/Basic/OpenCLOptions.h
39 ↗(On Diff #68677)

This argument is used when Ext is not prefixed by '+' or '-'.

I think we need two more tests for concatenating and overriding the option, e.g

-cl-ext=-cl_khr_fp64 -cl-ext=+cl_khr_fp64

and

-cl-ext=-cl_khr_fp64,+cl_khr_fp64
include/clang/Basic/OpenCLOptions.h
39 ↗(On Diff #72143)

Better add a comments for this function about its semantics, i.e., if Ext does not starts with +/-, it is enabled/disabled by \p Enable, otherwise +/- overrides \p Enable, since this is not obvious.

Anastasia added inline comments.Sep 27 2016, 11:23 AM
include/clang/Basic/OpenCLOptions.h
39 ↗(On Diff #72143)

Indeed, generally it would be nice to add a comment explaining the purpose of this functions. I don't think the name is descriptive enough.

include/clang/Driver/Options.td
394 ↗(On Diff #72143)

I would see it as cc1 option instead to avoid confusions on its intension.

395 ↗(On Diff #72143)

Could we also add a short statement, that +/- are used to turn the extesions on and off.

lib/Basic/Targets.cpp
1882 ↗(On Diff #72143)

Is this really target specific? I feel this should rather go into common code.

asavonic updated this revision to Diff 73443.Oct 4 2016, 3:29 AM
  • Describe OpenCLOptions::set() function
  • Move -cl-ext option to cc1
  • Reword -cl-ext option help
  • Move -cl-ext handling out of target-specific code
  • Add two more test cases regarding -cl-ext option
yaxunl accepted this revision.Oct 4 2016, 8:52 AM
yaxunl edited edge metadata.

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 4 2016, 8:52 AM
joey edited edge metadata.Oct 24 2016, 3:54 AM

Two minor comments, but otherwise LGTM!

include/clang/Basic/OpenCLOptions.h
33 ↗(On Diff #73443)

This comment needs to be changed, to reflect that they are now all enabled or disabled.

include/clang/Basic/TargetInfo.h
992 ↗(On Diff #73443)

Can you put a space around the ':'.

asavonic updated this revision to Diff 75568.Oct 24 2016, 4:10 AM
asavonic edited edge metadata.
  • Fix comments and code formatting
This revision was automatically updated to reflect the committed changes.