This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenCL] Allow pointers in structs as kernel arguments from 2.0
ClosedPublic

Authored by Ayal on Feb 12 2023, 9:40 AM.

Details

Summary

Structs that contain global or local pointers can be passed as kernel
arguments starting OpenCL v2.0 which introduces shared virtual memory.

Trying to revive a patch considered in D49723

Diff Detail

Event Timeline

Ayal created this revision.Feb 12 2023, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2023, 9:40 AM
Ayal requested review of this revision.Feb 12 2023, 9:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2023, 9:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I feel that originally pointers were disallowed because they create the same issue as size_t and etc as their size is implementation depended but the same logic applies to images and other types that are even more implementation depended. Overall this bit of the spec is very inconsistent so I don't mind if we change the behavior to be whatever we find more helpful. However I would encourage to submit an issue to OpenCL-Docs to point out this inconsistency.

clang/lib/Sema/SemaDecl.cpp
9493

To be honest I feel like it was a bug fix? Do you happen to have any record of the fix?

9494

I think it should be possible to merge this with if below to avoid condition duplication.

Ayal added a comment.Feb 12 2023, 3:18 PM

I feel that originally pointers were disallowed because they create the same issue as size_t and etc as their size is implementation depended but the same logic applies to images and other types that are even more implementation depended. Overall this bit of the spec is very inconsistent so I don't mind if we change the behavior to be whatever we find more helpful. However I would encourage to submit an issue to OpenCL-Docs to point out this inconsistency.

Yeah, pointer sizes need to match, pointers should be passed to clSetKernelExecInfo, and SVM should be employed, which the spec should clarify. In any case the quoted restriction "s6.9.p" of https://registry.khronos.org/OpenCL/sdk/1.2/docs/man/xhtml/restrictions.html justifying the SemA enforcement is clearly absent in https://registry.khronos.org/OpenCL/sdk/2.0/docs/man/xhtml/restrictions.html.

clang/lib/Sema/SemaDecl.cpp
9493

Not sure what was the alleged bug nor its fix, but in any case I have no record thereof..

9494

Sure, but that trades one duplication for another, rather than clearly separating the early-continue case early?

if (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam) {
  if (S.getLangOpts().getOpenCLCompatibleVersion() > 120)
    continue;
  S.Diag(Param->getLocation(),
         diag::err_record_with_pointers_kernel_param)
    << PT->isUnionType()
    << PT;
} else if (ParamType == InvalidAddrSpacePtrKernelParam) {
  S.Diag(Param->getLocation(),
         diag::err_record_with_pointers_kernel_param)
    << PT->isUnionType()
    << PT;
} else {
  S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
Anastasia added inline comments.Feb 13 2023, 2:47 AM
clang/lib/Sema/SemaDecl.cpp
9494

I am mainly thinking in terms of maintenance if for example someone fixes one if and forgets another. Or if ifs will get separated by some other code and then it is not easy to see that the same thing is handled differently in OpenCL versions.

Unfortunately we have a lot of those cases, I know this function has early exists but it is not a common style.

I was talking about something like:

if (((S.getLangOpts().getOpenCLCompatibleVersion() <= 120) &&
    (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam)) ||
          ParamType == InvalidAddrSpacePtrKernelParam)

It would also be ok to separate InvalidAddrSpacePtrKernelParam since it's handling different feature.

yaxunl added inline comments.Feb 13 2023, 7:53 AM
clang/test/SemaOpenCL/invalid-kernel-parameters.cl
90

we should not limit the tests to CL1.2. We should test them with 2.0 to make sure there is no diagnostics.

To differentiate between 1.2 and 2.0 you can use -verify=ocl12 and -verify=ocl20.

same as below

Ayal updated this revision to Diff 497128.Feb 13 2023, 3:19 PM

Updated version merges the if's and checks tests for both 1.2 and 2.0.

Ayal added inline comments.Feb 13 2023, 3:24 PM
clang/test/SemaOpenCL/invalid-kernel-parameters.cl
90

Using -verify=ocl12 and -verify=ocl20 should probably be done separately as an NFC patch as it involves many changes IIUC.

The expect's were placed under #ifdef instead (and only them), following e.g. existing lines 8-12.

yaxunl added inline comments.Feb 13 2023, 6:47 PM
clang/test/SemaOpenCL/invalid-kernel-parameters.cl
90

you can use -verify=expected,ocl12 for OCL1.2 and -verify for OCL2.0 then the common diagnostics are kept as 'expected-*' and unchanged. you only need to change the ocl1.2 specific diagnostics to 'ocl12-*'.

For example, https://github.com/llvm/llvm-project/blob/main/clang/test/SemaOpenCL/atomic-ops.cl

Ayal updated this revision to Diff 497317.Feb 14 2023, 7:13 AM

Use -verify=expected,ocl12 instead of #ifdef'ing the test to check diagnostics emitted for 1.2 but not emitted for 2.0.

Ayal added inline comments.Feb 14 2023, 7:17 AM
clang/test/SemaOpenCL/invalid-kernel-parameters.cl
90

Ah, ok, done, thanks!

(Note that atomic-ops.cl uses only expected's, some of which are placed under #if, as done elsewhere in this test file.)

Anastasia added inline comments.Feb 15 2023, 10:00 AM
clang/lib/Sema/SemaDecl.cpp
9494

Sorry I have forgotten that this part of the code is expected to handle the diagnostics only. The decision that the kernel parameter is wrong is done in getOpenCLKernelParameterType. I think you should alter the conditions there to check for OpenCL version and avoid classifying cases you care about as PtrKernelParam or PtrPtrKernelParam. Then here you wouldn't need this extra if/continue block.

Ayal added inline comments.Feb 15 2023, 3:22 PM
clang/lib/Sema/SemaDecl.cpp
9494

Hmm, would that be better? This part of the code, namely checkIsValidOpenCLKernelParameter(), does check the validity of arguments classified by getOpenCLKernelParameterType() in addition to handling diagnostics. E.g., the first case above decides that arguments of pointer-to-pointer type are wrong along with proper diagnostics for OpenCL 1.x while allowing them for other OpenCL versions.

Struct arguments are simply classified as records by getOpenCLKernelParameterType(), whereas this part of the code traverses each struct and calls getOpenCLKernelParameterType() on each field - the latter seems unaware if it was invoked on a struct field or not? If it is (made) aware, it could indeed return a (new kind of?) invalid type instead of pointer type for OpenCL 1.x - how would the right err_record_with_pointers_kernel_param diagnostics then be handled? If desired, such refactoring should probably be done independent of this fix?

Anastasia added inline comments.Feb 19 2023, 12:27 PM
clang/lib/Sema/SemaDecl.cpp
9494

That's right there is a mix of everything as I think this code has deviated from its original idea, but I still think it's cleaner to avoid doing the same kind of checks in multiple places.

Overall I find this code a bit over-engineered, this is maybe why it went off track. So some refactoring would indeed be helpful. However I am not sure whether it's better to continue the same route or try to simplify the design by just adding separate functions for each error case that would detect that the type is of a certain kind e.g a pointer or a pointer with wrong address space and then also provide the diagnostic straight away . I feel this would match better the rest of the diagnostic handling in clang but might result in slightly more helper functions or duplication of code.

Ayal updated this revision to Diff 502855.Mar 6 2023, 4:35 PM

It's hard for getOpenCLKernelParameterType() to detect and diagnose invalid pointer cases w/o context (of an enclosing struct or not), but it's easy to detect valid pointer cases for v2.0+ and return ValidKernelParam.

Rebased.

Ayal added inline comments.Mar 6 2023, 4:40 PM
clang/lib/Sema/SemaDecl.cpp
9494

Tried to avoid doing the same checks in multiple places by having getOpenCLKernelParameterType() identify valid pointer cases as ValidKernelParam, please see above.

Does this look ok? If some (NFC) refactoring is still desired, better apply it after this bug fix?

Anastasia accepted this revision.Mar 12 2023, 3:22 PM

LGTM! Thanks

This revision is now accepted and ready to land.Mar 12 2023, 3:22 PM
This revision was landed with ongoing or failed builds.Mar 13 2023, 10:04 AM
This revision was automatically updated to reflect the committed changes.