This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Check for invalid kernel arguments in array types
ClosedPublic

Authored by asavonic on Jul 24 2018, 4:32 AM.

Details

Summary

OpenCL specification forbids use of several types as kernel
arguments. This patch improves existing diagnostic to look through
arrays.

Diff Detail

Repository
rC Clang

Event Timeline

asavonic created this revision.Jul 24 2018, 4:32 AM
Anastasia accepted this revision.Jul 24 2018, 4:56 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Jul 24 2018, 4:56 AM
yaxunl accepted this revision.Jul 24 2018, 5:41 AM

LGTM

Btw, has this restriction been removed from CL 2.0?

asavonic updated this revision to Diff 157219.Jul 25 2018, 4:06 AM

Moved a chunk from https://reviews.llvm.org/D49725; added 2 more tests.

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

lib/Sema/SemaDecl.cpp
8187

Do we need to assert PT here too? It doesn't seem to be modified in this loop...

asavonic updated this revision to Diff 157685.Jul 27 2018, 7:22 AM
asavonic marked an inline comment as done.

Fix assert.

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

Right, and it seems that pointers in struct arguments should also be legal in CL2.0.
I'll submit another patch to remove this check for CL2.0.

lib/Sema/SemaDecl.cpp
8187

We should check for a field instead. Thanks!

asavonic updated this revision to Diff 157688.Jul 27 2018, 7:26 AM

Rollback accidentally squashed commit.

Anastasia accepted this revision.Jul 27 2018, 8:38 AM

LGTM! Thanks!

yaxunl accepted this revision.Jul 27 2018, 8:28 PM

LGTM. Thanks.

This revision was automatically updated to reflect the committed changes.
Ayal added a subscriber: Ayal.Feb 9 2023, 2:05 PM

Btw, has this restriction been removed from CL 2.0?

No, it applies for CL2.0 as well.

It seems however the restriction on pointer to pointer was removed (see s6.9.a last item) in CL2.0.

Right, and it seems that pointers in struct arguments should also be legal in CL2.0.
I'll submit another patch to remove this check for CL2.0.

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 2:05 PM
Herald added a subscriber: Naghasan. · View Herald Transcript

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

Yeah, the patch got lost somewhere, I'm sorry...

TBH, I don't know why pointers in structs or arrays were disallowed in the first place. Even OpenCL 1.2 does not say it explicitly, although there is a bit suspicious point in s6.9.p:

Arguments to kernel functions that are declared to be a struct or union do not allow
OpenCL objects to be passed as elements of the struct or union

It does not say "pointers", just "OpenCL objects". Sounds more like events or images to me, not pointers.

Ayal added a comment.Feb 12 2023, 9:47 AM

This is admittedly a couple of years old by now, but wonder about that other intended patch - Clang still seems to consider pointers in struct arguments to be illegal in CL2.0 (and CL3.0) - please see https://godbolt.org/z/E87z66h1d

Yeah, the patch got lost somewhere, I'm sorry...

Sure, trying to resurrect it in a comment above, and in D143849.

TBH, I don't know why pointers in structs or arrays were disallowed in the first place. Even OpenCL 1.2 does not say it explicitly, although there is a bit suspicious point in s6.9.p:

Arguments to kernel functions that are declared to be a struct or union do not allow
OpenCL objects to be passed as elements of the struct or union

It does not say "pointers", just "OpenCL objects". Sounds more like events or images to me, not pointers.

Yeah, there's room to improve clarity of the spec. Passing pointers indirectly requires them to point to same/share address space, as provided by SVM starting in OpenCL2.0.

lib/Sema/SemaDecl.cpp
8218

Suggest to (rebase and) update this as follows, based on
https://registry.khronos.org/OpenCL/sdk/2.0/docs/man/xhtml/restrictions.html

// union. This restriction was lifted in OpenCL v2.0 with the introduction
// of SVM.
if (S.getLangOpts().getOpenCLCompatibleVersion() > 120 &&
    (ParamType == PtrKernelParam || ParamType == PtrPtrKernelParam))
  continue;