This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Add support of __opencl_c_device_enqueue feature macro.
ClosedPublic

Authored by azabaznov on Dec 13 2021, 7:46 AM.

Details

Summary

This feature requires support of opencl_c_generic_address_space and
opencl_c_program_scope_global_variables so diagnostics for that is provided as well.

Diff Detail

Unit TestsFailed

Event Timeline

azabaznov created this revision.Dec 13 2021, 7:46 AM
azabaznov requested review of this revision.Dec 13 2021, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 7:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This feature requires support of opencl_c_generic_address_space and

opencl_c_program_scope_global_variables so diagnostics for that is provided as well.

Do we have a spec issue for it or a PR? It would be good to list it here since it is not yet published in the registry.

clang/lib/Sema/Sema.cpp
329

Here it should be sufficient to just check getLangOpts().Blocks only wothout the versions.

clang/test/Misc/opencl-c-3.0.incorrect_options.cl
21

I can't remember if we have discussed this already, but could we use -verify for these errors?

clang/test/SemaOpenCL/invalid-device-enqueue-types-cl3.0.cl
5

I know that many test have prefix "invalid" but I feel we have failed to establish the meaning for it because most of the tests in Sema are testing some sort of invalid behavior. But also here I feel that we should test that enqueue_kernel is not supported?

Do you think we chould merge this testing together with SemaOpenCL/cl20-device-side-enqueue.cl with some filename renaming?

Technically we should do the same testing even for CL1.x versions...

clang/test/SemaOpenCL/storageclass.cl
2

These lines are getting a bit longer. Do we actually need to set -__opencl_c_device_enqueue for this test? Same for some other tests...

azabaznov updated this revision to Diff 395390.Dec 20 2021, 1:18 AM

Simplified running lines in tests, use 'verify' when validating features, simplify condition in Sema

azabaznov marked 3 inline comments as done.Dec 20 2021, 1:22 AM
azabaznov added inline comments.
clang/test/SemaOpenCL/invalid-device-enqueue-types-cl3.0.cl
5

I think it can. But enqueu_kernel is a LangBuiltin and still not supported yet. Support for LangBuiltins is going to be added in a separate patch: with this patch all of the features that affect language built-ins are supported.

Anastasia added inline comments.Dec 20 2021, 9:15 AM
clang/test/Misc/opencl-c-3.0.incorrect_options.cl
21

We should be able to remove FileCheck and replace CHECK directives with something like:
//expected-error@*{{options cl_khr_fp64 and __opencl_c_fp64 are set to different values}}

clang/test/SemaOpenCL/invalid-device-enqueue-types-cl3.0.cl
5

Ok, then would it still work if we merge this functionality SemaOpenCL/cl20-device-side-enqueue.cl and the rest can be added later on...

clang/test/SemaOpenCL/storageclass.cl
2

ping

azabaznov added inline comments.Jan 12 2022, 2:18 AM
clang/test/Misc/opencl-c-3.0.incorrect_options.cl
21

I don't think we can test it like this because there is different output for each invalid combination, so we need to check them with label.

clang/test/SemaOpenCL/invalid-device-enqueue-types-cl3.0.cl
5

It's difficult to refactor that test, since it relies on the fact that device enqueue feature is supported and checks for incorrect constructs. We can't enable it for 3.0 now since language built-ins (enqueue_kernel etc.) are not supported yet.

clang/test/SemaOpenCL/storageclass.cl
2

Yeah, we need that here because we are turning off generic AS and PSV in this test. Note that __opencl_c_device_enqueue is turned off with -cl-ext=-all.

azabaznov updated this revision to Diff 399269.Jan 12 2022, 2:19 AM

Rebase, remove extra 'verify'

Anastasia added inline comments.Jan 24 2022, 7:45 AM
clang/test/Misc/opencl-c-3.0.incorrect_options.cl
21

We normally use macros to guard the expected errors but however, this is a better fit for a refactoring patch... so I am ok if it's done separately...

However, don't you need to add

//expected-no-diagnostics

directive?

clang/test/SemaOpenCL/storageclass.cl
2

since this test doesn't check anything for __opencl_c_device_enqueue why do we need to switch this off explicitly?

azabaznov updated this revision to Diff 402873.Jan 25 2022, 5:39 AM

Add expected-no-diagnostics, fix misprint in test.

azabaznov marked an inline comment as done.Jan 25 2022, 5:40 AM
azabaznov added inline comments.
clang/test/SemaOpenCL/storageclass.cl
2

In this test PSV and generic address space features are alternately turned off. Since they are required to by device enqueue, it's required to turn device enqueue as well.

Anastasia accepted this revision.Jan 27 2022, 2:54 AM

LGTM! Thanks

This revision is now accepted and ready to land.Jan 27 2022, 2:54 AM
This revision was landed with ongoing or failed builds.Jan 27 2022, 3:26 AM
This revision was automatically updated to reflect the committed changes.