Skip to content

[NFC][SYCL] Simplify the error checking of arrays by only visiting 1x. #2344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 20, 2020
Merged

[NFC][SYCL] Simplify the error checking of arrays by only visiting 1x. #2344

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0382217
[NFC][SYCL] Make Visitor functions private, other than the entry points
Aug 19, 2020
f5bc925
[NFC][SYCL] Simplify the error checking of arrays by only visiting 1x.
Aug 19, 2020
62a3d57
Clang format fix
Aug 19, 2020
eb83c13
Merge branch 'sycl' into only_visit-first_array
Aug 19, 2020
d67a6f8
Replace 'if' with 'assert' for zero size arrays
Aug 20, 2020
f051ae8
Remove duplicate lineP
Aug 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 51 additions & 18 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,8 +832,8 @@ class KernelObjVisitor {

// Implements the 'for-each-visitor' pattern.
template <typename... Handlers>
void VisitElement(CXXRecordDecl *Owner, FieldDecl *ArrayField,
QualType ElementTy, Handlers &... handlers) {
void VisitElementImpl(CXXRecordDecl *Owner, FieldDecl *ArrayField,
QualType ElementTy, Handlers &... handlers) {
if (Util::isSyclAccessorType(ElementTy))
KF_FOR_EACH(handleSyclAccessorType, ArrayField, ElementTy);
else if (Util::isSyclStreamType(ElementTy))
Expand All @@ -854,6 +854,16 @@ class KernelObjVisitor {
KF_FOR_EACH(handleScalarType, ArrayField, ElementTy);
}

template <typename... Handlers>
void VisitFirstElement(CXXRecordDecl *Owner, FieldDecl *ArrayField,
QualType ElementTy, Handlers &... handlers) {
VisitElementImpl(Owner, ArrayField, ElementTy, handlers...);
}

template <typename... Handlers>
void VisitNthElement(CXXRecordDecl *Owner, FieldDecl *ArrayField,
QualType ElementTy, Handlers &... handlers);

template <typename... Handlers>
void VisitArrayElements(FieldDecl *FD, QualType FieldTy,
Handlers &... handlers) {
Expand All @@ -863,17 +873,35 @@ class KernelObjVisitor {
QualType ET = CAT->getElementType();
int64_t ElemCount = CAT->getSize().getSExtValue();
std::initializer_list<int>{(handlers.enterArray(), 0)...};
for (int64_t Count = 0; Count < ElemCount; Count++) {
VisitElement(nullptr, FD, ET, handlers...);

assert(ElemCount > 0 && "SYCL prohibits 0 sized arrays");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, you crash the compiler when a user uses a array of size 0?
And what when not compiled with assertions?
What if this happens by meta-programming on a kernel which is not executed anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prohibit size-zero arrays. If the user attempts to use a size-zero array in a kernel, they get a diagnostic.

VisitFirstElement(nullptr, FD, ET, handlers...);
(void)std::initializer_list<int>{(handlers.nextElement(ET), 0)...};

for (int64_t Count = 1; Count < ElemCount; Count++) {
VisitNthElement(nullptr, FD, ET, handlers...);
(void)std::initializer_list<int>{(handlers.nextElement(ET), 0)...};
}

(void)std::initializer_list<int>{
(handlers.leaveArray(FD, ET, ElemCount), 0)...};
}

// Parent contains the FieldDecl or CXXBaseSpecifier that was used to enter
// the Wrapper structure that we're currently visiting. Owner is the parent
// type (which doesn't exist in cases where it is a FieldDecl in the
// 'root'), and Wrapper is the current struct being unwrapped.
template <typename ParentTy, typename... Handlers>
void VisitRecord(const CXXRecordDecl *Owner, ParentTy &Parent,
const CXXRecordDecl *Wrapper, Handlers &... handlers);
const CXXRecordDecl *Wrapper, Handlers &... handlers) {
(void)std::initializer_list<int>{
(handlers.enterStruct(Owner, Parent), 0)...};
VisitRecordHelper(Wrapper, Wrapper->bases(), handlers...);
VisitRecordHelper(Wrapper, Wrapper->fields(), handlers...);
(void)std::initializer_list<int>{
(handlers.leaveStruct(Owner, Parent), 0)...};
}

template <typename ParentTy, typename... Handlers>
void VisitUnion(const CXXRecordDecl *Owner, ParentTy &Parent,
const CXXRecordDecl *Wrapper, Handlers &... handlers);
Expand Down Expand Up @@ -988,25 +1016,13 @@ class KernelObjVisitor {
}
#undef KF_FOR_EACH
};
// Parent contains the FieldDecl or CXXBaseSpecifier that was used to enter
// the Wrapper structure that we're currently visiting. Owner is the parent
// type (which doesn't exist in cases where it is a FieldDecl in the
// 'root'), and Wrapper is the current struct being unwrapped.
template <typename ParentTy, typename... Handlers>
void KernelObjVisitor::VisitRecord(const CXXRecordDecl *Owner, ParentTy &Parent,
const CXXRecordDecl *Wrapper,
Handlers &... handlers) {
(void)std::initializer_list<int>{(handlers.enterStruct(Owner, Parent), 0)...};
VisitRecordHelper(Wrapper, Wrapper->bases(), handlers...);
VisitRecordHelper(Wrapper, Wrapper->fields(), handlers...);
(void)std::initializer_list<int>{(handlers.leaveStruct(Owner, Parent), 0)...};
}

// A base type that the SYCL OpenCL Kernel construction task uses to implement
// individual tasks.
class SyclKernelFieldHandlerBase {
public:
static constexpr const bool VisitUnionBody = false;
static constexpr const bool VisitNthElement = true;
// Mark these virtual so that we can use override in the implementer classes,
// despite virtual dispatch never being used.

Expand Down Expand Up @@ -1115,6 +1131,21 @@ void KernelObjVisitor::VisitUnion(const CXXRecordDecl *Owner, ParentTy &Parent,
HandlerFilter<Handlers::VisitUnionBody, Handlers>(handlers).Handler...);
}

template <typename... Handlers>
void KernelObjVisitor::VisitNthElement(CXXRecordDecl *Owner,
FieldDecl *ArrayField,
QualType ElementTy,
Handlers &... handlers) {
// Don't continue descending if none of the handlers 'care'. This could be 'if
// constexpr' starting in C++17. Until then, we have to count on the
// optimizer to realize "if (false)" is a dead branch.
if (AnyTrue<Handlers::VisitNthElement...>::Value)
VisitElementImpl(
Owner, ArrayField, ElementTy,
HandlerFilter<Handlers::VisitNthElement, Handlers>(handlers)
.Handler...);
}

// A type to check the validity of all of the argument types.
class SyclKernelFieldChecker : public SyclKernelFieldHandler {
bool IsInvalid = false;
Expand Down Expand Up @@ -1237,6 +1268,7 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler {
public:
SyclKernelFieldChecker(Sema &S)
: SyclKernelFieldHandler(S), Diag(S.getASTContext().getDiagnostics()) {}
static constexpr const bool VisitNthElement = false;
bool isValid() { return !IsInvalid; }

bool handleReferenceType(FieldDecl *FD, QualType FieldTy) final {
Expand Down Expand Up @@ -1285,6 +1317,7 @@ class SyclKernelUnionChecker : public SyclKernelFieldHandler {
: SyclKernelFieldHandler(S), Diag(S.getASTContext().getDiagnostics()) {}
bool isValid() { return !IsInvalid; }
static constexpr const bool VisitUnionBody = true;
static constexpr const bool VisitNthElement = false;

bool checkType(SourceLocation Loc, QualType Ty) {
if (UnionCount) {
Expand Down