Page MenuHomePhabricator

[SYCL] Add support for sycl_special_class attribute
Needs ReviewPublic

Authored by zahiraam on Nov 23 2021, 3:06 PM.

Details

Summary

Special classes such as accessor, sampler, and stream need additional implementation when they are passed from host to device.
This patch is adding a new attribute “sycl_special_class” used to mark SYCL classes/struct that need the additional compiler handling.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

zahiraam created this revision.Nov 23 2021, 3:06 PM
zahiraam requested review of this revision.Nov 23 2021, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 3:06 PM
zahiraam retitled this revision from Add support for sycl_special_class attribute to [SYCL] Add support for sycl_special_class attribute.Dec 6 2021, 12:27 PM

Just a few comments

clang/include/clang/Basic/AttrDocs.td
415–417

I think this is a bit hard to follow if you don't know the SYCL kernel lowering process. Small suggestion to try to make this more self contained.

clang/lib/Sema/SemaDeclAttr.cpp
8265

Don't we need to have a validation for at least the __init function ? The doc says it is mandatory.

zahiraam updated this revision to Diff 393278.Dec 9 2021, 1:43 PM
zahiraam marked an inline comment as done.
zahiraam marked an inline comment as done.
zahiraam updated this revision to Diff 393532.Dec 10 2021, 10:25 AM
Fznamznon added inline comments.Dec 17 2021, 1:07 AM
clang/lib/Sema/SemaDecl.cpp
9125

In our downstream we have not only __init, but __init_esimd as well there. I wonder how this case should be handled?

16683–16691

Code style correction.

clang/lib/Sema/SemaDeclAttr.cpp
7840

I wonder if this check is enforced by this line in Attr.td:

let Subjects = SubjectList<[CXXRecord]>;

I would assume that it is.

clang/test/SemaSYCL/special-class-attribute.cpp
22

Did you mean struct keyword here?

32

Is this one needed?

44

Will detection of __init method work correctly if it is defined outside of the class?
Maybe we should also mention that it should be not be defined in a separate library.

zahiraam updated this revision to Diff 395176.Dec 17 2021, 11:21 AM
zahiraam marked 5 inline comments as done.
zahiraam added inline comments.
clang/lib/Sema/SemaDecl.cpp
9125

I think the presence of method __init_esimd implies the presence of __init method so we could eventually check first for the presence of __init_esimd and if not present check for the presence of __init?

clang/test/SemaSYCL/special-class-attribute.cpp
44

Both init and finalize must be defined inside the class/struct.