This is an archive of the discontinued LLVM Phabricator instance.

[SYCL] Add support for sycl_special_class attribute
ClosedPublic

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

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
8305

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
9173

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

16736–16751

Code style correction.

clang/lib/Sema/SemaDeclAttr.cpp
7880

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
9173

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.

bader accepted this revision.Jan 24 2022, 9:42 AM

LGTM, just one suggestion.
It would be great to get @aaron.ballman approve too.

clang/lib/Sema/SemaDecl.cpp
16743

I think we might want to check that there is only one member function with __init name to avoid ambiguity with building kernel parameters.

This revision is now accepted and ready to land.Jan 24 2022, 9:42 AM

Thanks! Mostly just minor nits from me.

clang/include/clang/AST/DeclCXX.h
1142
clang/include/clang/Basic/AttrDocs.td
415
416
420
421
422

This still isn't quite right -- I don't know how to read "additional information required".

424
425
428
429
432
clang/lib/Sema/SemaDecl.cpp
9174–9175
16739–16740

I think we can assert there is a definition given that this is called from "ActOnTagFinishDefinition".

zahiraam marked 11 inline comments as done.Jan 24 2022, 12:00 PM
zahiraam added inline comments.
clang/lib/Sema/SemaDecl.cpp
16739–16740

@aaron.ballman Not sure what you mean by "that this is called from". We are inside the ActOnTagFinishDefinition function.

aaron.ballman added inline comments.Jan 24 2022, 12:12 PM
clang/lib/Sema/SemaDecl.cpp
16739–16740

Sorry for being unclear -- yes, we're inside ActOnTagFinishDefinition() and we just called FinishClass(), so I don't think we need to test if (Def). Instead, I think we can do:

const TagDecl *Def = RD->getDefinition();
assert(Def && "Expected to have a completed definition");
if (!Def->hasInitMethod())
zahiraam updated this revision to Diff 402662.Jan 24 2022, 2:08 PM
zahiraam marked 4 inline comments as done.
keryell accepted this revision.Jan 24 2022, 6:52 PM

That looks good.

clang/include/clang/Basic/AttrDocs.td
459
zahiraam updated this revision to Diff 402850.Jan 25 2022, 4:39 AM
zahiraam marked an inline comment as done.
aaron.ballman accepted this revision.Jan 25 2022, 6:38 AM

LGTM aside from some small nits.

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

I love the Oxford comma and am not afraid to admit it in public. :-D

422
clang/test/SemaSYCL/special-class-attribute-on-non-sycl.cpp
12
clang/test/SemaSYCL/special-class-attribute.cpp
6

Can you correct this entire file not to have the trailing semicolons?

Naghasan accepted this revision.Jan 25 2022, 6:43 AM

LGTM, thanks for the work

zahiraam updated this revision to Diff 402900.Jan 25 2022, 7:05 AM
zahiraam marked 3 inline comments as done.

Thanks for the reviews.

MaskRay closed this revision.EditedJan 28 2022, 2:34 PM
MaskRay added a subscriber: MaskRay.

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

I've pushed a fix for it in a10ff373ddfa82a67c580a87f6258aa6ab8dd595 and the sphinx builder is happy again.

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

I've pushed a fix for it in a10ff373ddfa82a67c580a87f6258aa6ab8dd595 and the sphinx builder is happy again.

Thanks @aaron.ballman !