Page MenuHomePhabricator

[OpenCL] Forbid size dependent types used as kernel arguments
ClosedPublic

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

Details

Summary

Size_t, intptr_t, uintptr_t and ptrdiff_t cannot be used as kernel
arguments, according to OpenCL Specification s6.9k:

The size in bytes of these types are implementation-defined and in
addition can also be different for the OpenCL device and the host
processor making it difficult to allocate buffer objects to be passed
as arguments to a kernel declared as pointer to these types.

Diff Detail

Repository
rL LLVM

Event Timeline

asavonic created this revision.Jul 24 2018, 4:34 AM

This patch also adds check for array of structs. Can you include this in title or split to a separate patch?

lib/Sema/SemaDecl.cpp
8065 ↗(On Diff #157005)

Can we record the real size_t/intptr_t/etc typedef and later on emit a note for it? It helps user to locate the culprit typedef in case of multiple typedefs.

8189 ↗(On Diff #157005)

Can we have a test for this change? e.g. an array of structs

Anastasia added inline comments.Jul 24 2018, 6:52 AM
lib/Sema/SemaDecl.cpp
8186 ↗(On Diff #157005)

I am a bit confused about this comment, do you mean a PointerType to a RecordType or an ArrayType of a RecordType?

8186 ↗(On Diff #157005)

Also is there any test case covering this change?

8189 ↗(On Diff #157005)

I am wondering if PT->getPointeeOrArrayElementType() is nullptr? Do we need to add an extra check?

asavonic updated this revision to Diff 157222.Jul 25 2018, 4:11 AM

Added a diagnostic note for typedefs; moved unrelated changes to
https://reviews.llvm.org/D49723.

asavonic marked 5 inline comments as done.Jul 25 2018, 4:19 AM

This patch also adds check for array of structs. Can you include this in title or split to a separate patch?

I'm sorry, this change with arrays should actually go into https://reviews.llvm.org/D49723. Moved it there.

lib/Sema/SemaDecl.cpp
8065 ↗(On Diff #157005)

I changed the patch to emit a note for all typedefs involved in
InvalidKernelParam. This way it works not only for size_t types, but
also for other invalid types which were typedef'ed.

8186 ↗(On Diff #157005)

ArrayType of a RecordType. Fixed.

8186 ↗(On Diff #157005)

I added 2 tests for this change in https://reviews.llvm.org/D49723.

8189 ↗(On Diff #157005)
8189 ↗(On Diff #157005)

It should not return null, unless the PT is null:
https://clang.llvm.org/doxygen/Type_8h_source.html#l06487

asavonic marked 5 inline comments as done.Jul 25 2018, 4:20 AM
Anastasia added inline comments.Jul 26 2018, 2:54 AM
lib/Sema/SemaDecl.cpp
8267 ↗(On Diff #157222)

Should this bit go into https://reviews.llvm.org/D49725?

asavonic marked an inline comment as done.Jul 26 2018, 5:57 AM
asavonic added inline comments.
lib/Sema/SemaDecl.cpp
8267 ↗(On Diff #157222)

Right, thank you!

asavonic marked an inline comment as done.Jul 26 2018, 5:58 AM
yaxunl accepted this revision.Jul 26 2018, 8:17 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 26 2018, 8:17 AM
Anastasia accepted this revision.Jul 27 2018, 6:26 AM

LGTM! Thanks!

This revision was automatically updated to reflect the committed changes.

FYI: @asavonic, the email address you have associated with your commit id is AlexeySotkin@/etc/mailname which is getting stuck in the moderation queue as not being signed up to the mailing list. You may want to correct your svn information as I am not certain what our list software will think of that domain.

AlexeySotkin added a comment.EditedAug 1 2018, 3:16 AM

FYI: @asavonic, the email address you have associated with your commit id is AlexeySotkin@/etc/mailname which is getting stuck in the moderation queue as not being signed up to the mailing list. You may want to correct your svn information as I am not certain what our list software will think of that domain.

Hi @aaron.ballman. It seems that this issue has broken mirroring with github repo. How I can fix it? Thanks.