Skip to content

[SYCL] Changes to visitor model in preparation of array support. #1764

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
Jun 6, 2020
Merged

[SYCL] Changes to visitor model in preparation of array support. #1764

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
95ea801
[SYCL] Changes to visitor model in preparation of array support.
rdeodhar May 27, 2020
c09dae4
Added conditional execution of handlers.
rdeodhar May 30, 2020
f1161d6
Changes to visitor model to run handlers conditionally.
rdeodhar Jun 2, 2020
97ddadb
Corrections to returning validity.
rdeodhar Jun 4, 2020
a16091a
Corrected file upload.
rdeodhar Jun 4, 2020
2a36ef6
Merge branch 'sycl' of https://github.com/otcshare/llvm into akp1
rdeodhar Jun 5, 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
166 changes: 116 additions & 50 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -686,22 +686,47 @@ static void VisitAccessorWrapper(CXXRecordDecl *Owner, ParentTy &Parent,
CXXRecordDecl *Wrapper,
Handlers &... handlers);

template <typename RangeTy, typename... Handlers>
static void VisitField(CXXRecordDecl *Owner, RangeTy Item, QualType ItemTy,
Handlers &... handlers) {
if (Util::isSyclAccessorType(ItemTy)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use curley brackets for single line if-statements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

(void)std::initializer_list<int>{
(handlers.handleSyclAccessorType(Item, ItemTy), 0)...};
} else if (Util::isSyclStreamType(ItemTy))
(void)std::initializer_list<int>{
(handlers.handleSyclStreamType(Item, ItemTy), 0)...};
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This else body shouldn't be necessary, make the two checks top-level. Additionally, please leave the existing order (where structure/class type happens right after accessor/stream) and put the array at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

if (ItemTy->isArrayType()) {
VisitArrayElements(Item, ItemTy, handlers...);
} else if (ItemTy->isStructureOrClassType()) {
VisitAccessorWrapper(Owner, Item, ItemTy->getAsCXXRecordDecl(),
handlers...);
}
}
}

template <typename RangeTy, typename... Handlers>
static void VisitArrayElements(RangeTy Item, QualType FieldTy,
Handlers &... handlers) {
const ConstantArrayType *CAT = cast<ConstantArrayType>(FieldTy);
Copy link
Contributor

Choose a reason for hiding this comment

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

When you do cast to ConstantArrayType you expect that FieldTy is ConstantArrayType, otherwise assertion will be triggered, so, is it safe to do cast here? I'm asking because VisitArrayElements is called after check on isArrayType, not on isConstantArrayType.

Copy link
Contributor

Choose a reason for hiding this comment

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

Later, he leaves this comment, which made me not comment on this. but it is still worth evaluating:
// C++ lambda capture already flags non-constant array types.
// Here, we check copyability.

There are 4 types of arrays:
Constant
This one is obvious.

DependentSized
I dont think we have to worry about this one, all instantiations are done before we get here, right?

Incomplete
You cannot actually capture an incomplete sized array type as far as I can tell. They aren't particularly easy to spell in C, and I think we prevent capturing them: https://godbolt.org/z/-oDsdz

Variable
This is the only one that I think is possible, but only as a reference type: https://godbolt.org/z/hEdiK-
Of course, VLAs in structs aren't supported in clang (with the pretty assertive message: fields must have a constant size: 'variable length array in structure' extension will never be supported)

That said, Do we prevent the reference case from making it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK SYCL spec allows only "by value" ([=] this one) capture for kernels, AFAIK we just don't diagnose if someone tried to use "by reference" capture.
This comment didn't satisfy me because kernel object can be defined not only as lambda but as user-defined callable object as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, then the VariableArraytype isn't a concern, though we should probably diagnose that (should we diagnose ALL ReferenceType fields?).

I'd not thought about other object types. From that perspective, only the incomplete type is of concern, we should likely diagnose that.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: This is another great use of the 'have visit methods return the boolean value'. We could/should stop descending once we report failure.

Copy link
Contributor

@Fznamznon Fznamznon May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I just forgot, but it seems we already diagnose ReferenceType fields, don't we?

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 810 in 08f8656

IsInvalid = Diag.Report(FD->getLocation(), diag::err_bad_kernel_param_type)

From that perspective, only the incomplete type is of concern, we should likely diagnose that.

Agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non-constant arrays could slip through via function objects. I've added a check.

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++) {
VisitField(nullptr, Item, ET, handlers...);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that 'visitField' could instead have its 'owner' be a generic declaration, so that the array type could pass that on instead. This would also allow offset-handling quite a bit more easily I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this. Let's wait until the handleArrayType bodies are filled in.

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

template <typename RangeTy, typename... Handlers>
static void VisitAccessorWrapperHelper(CXXRecordDecl *Owner, RangeTy Range,
Handlers &... handlers) {
for (const auto &Item : Range) {
QualType ItemTy = getItemType(Item);
if (Util::isSyclAccessorType(ItemTy))
(void)std::initializer_list<int>{
(handlers.handleSyclAccessorType(Item, ItemTy), 0)...};
else if (Util::isSyclStreamType(ItemTy)) {
VisitAccessorWrapper(Owner, Item, ItemTy->getAsCXXRecordDecl(),
handlers...);
(void)std::initializer_list<int>{
(handlers.handleSyclStreamType(Item, ItemTy), 0)...};
} else if (ItemTy->isStructureOrClassType())
VisitAccessorWrapper(Owner, Item, ItemTy->getAsCXXRecordDecl(),
handlers...);
(void)std::initializer_list<int>{(handlers.enterField(Owner, Item), 0)...};
VisitField(Owner, Item, ItemTy, handlers...);
(void)std::initializer_list<int>{(handlers.leaveField(Owner, Item), 0)...};
}
}

Expand All @@ -728,6 +753,8 @@ static void VisitRecordFields(RecordDecl::field_range Fields,
(void)std::initializer_list<int> { (handlers.FUNC(Field, FieldTy), 0)... }

for (const auto &Field : Fields) {
(void)std::initializer_list<int>{
(handlers.enterField(nullptr, Field), 0)...};
QualType FieldTy = Field->getType();

if (Util::isSyclAccessorType(FieldTy))
Expand All @@ -749,12 +776,15 @@ static void VisitRecordFields(RecordDecl::field_range Fields,
KF_FOR_EACH(handleReferenceType);
else if (FieldTy->isPointerType())
KF_FOR_EACH(handlePointerType);
else if (FieldTy->isArrayType())
else if (FieldTy->isArrayType()) {
KF_FOR_EACH(handleArrayType);
else if (FieldTy->isScalarType())
VisitArrayElements(Field, FieldTy, handlers...);
} else if (FieldTy->isScalarType())
KF_FOR_EACH(handleScalarType);
else
KF_FOR_EACH(handleOtherType);
(void)std::initializer_list<int>{
(handlers.leaveField(nullptr, Field), 0)...};
}
#undef KF_FOR_EACH
}
Expand All @@ -780,6 +810,7 @@ template <typename Derived> class SyclKernelFieldHandler {
virtual void handleStructType(FieldDecl *, QualType) {}
virtual void handleReferenceType(FieldDecl *, QualType) {}
virtual void handlePointerType(FieldDecl *, QualType) {}
virtual void handleArrayType(const CXXBaseSpecifier &, QualType) {}
virtual void handleArrayType(FieldDecl *, QualType) {}
virtual void handleScalarType(FieldDecl *, QualType) {}
// Most handlers shouldn't be handling this, just the field checker.
Expand All @@ -793,6 +824,17 @@ template <typename Derived> class SyclKernelFieldHandler {
virtual void leaveStruct(const CXXRecordDecl *, FieldDecl *) {}
virtual void enterStruct(const CXXRecordDecl *, const CXXBaseSpecifier &) {}
virtual void leaveStruct(const CXXRecordDecl *, const CXXBaseSpecifier &) {}

// The following are used for stepping through array elements.

virtual void enterField(const CXXRecordDecl *, const CXXBaseSpecifier &) {}
virtual void leaveField(const CXXRecordDecl *, const CXXBaseSpecifier &) {}
virtual void enterField(const CXXRecordDecl *, FieldDecl *) {}
virtual void leaveField(const CXXRecordDecl *, FieldDecl *) {}
virtual void enterArray(const CXXBaseSpecifier &) {}
virtual void enterArray() {}
virtual void nextElement(QualType) {}
virtual void leaveArray(QualType, int64_t) {}
};

// A type to check the validity of all of the argument types.
Expand All @@ -801,6 +843,43 @@ class SyclKernelFieldChecker
bool IsInvalid = false;
DiagnosticsEngine &Diag;

// Check whether the object is bit-wise copyable
bool copyableToKernel(const FieldDecl *FD, const QualType &FieldTy) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a 'check' type function, and thus should be 'checkCopyableToKernel'. With that, please invert the 'bool' (false is successful). Then, all of the Diagnostics.report, followed by a return changes to "return Diagnostics.Report", since that is implicitly convertable to bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

// C++ lambda capture already flags non-constant array types.
// Here, we check copyability.
if (FieldTy->isConstantArrayType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (FieldTy->isConstantArrayType()) {
if (const auto *CAT = dyn_cast<ConstantArrayType>(FieldTy)) {

And so fourth, do the same elsewhere if you're going to use the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

const ConstantArrayType *CAT = cast<ConstantArrayType>(FieldTy);
QualType ET = CAT->getElementType();
return copyableToKernel(FD, ET);
}
if (SemaRef.getASTContext().getLangOpts().SYCLStdLayoutKernelParams) {
if (!FieldTy->isStandardLayoutType()) {
SemaRef.getASTContext().getDiagnostics().Report(
FD->getLocation(), diag::err_sycl_non_std_layout_type)
<< FieldTy;
return false;
}
}
if (!FieldTy->isStructureOrClassType()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no curleys for single liners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

return true;
}
CXXRecordDecl *RD =
cast<CXXRecordDecl>(FieldTy->getAs<RecordType>()->getDecl());
if (!RD->hasTrivialCopyConstructor()) {
SemaRef.getASTContext().getDiagnostics().Report(
FD->getLocation(), diag::err_sycl_non_trivially_copy_ctor_dtor_type)
<< 0 << FieldTy;
return false;
}
if (!RD->hasTrivialDestructor()) {
SemaRef.getASTContext().getDiagnostics().Report(
FD->getLocation(), diag::err_sycl_non_trivially_copy_ctor_dtor_type)
<< 1 << FieldTy;
return false;
}
return true;
}

public:
SyclKernelFieldChecker(Sema &S)
: SyclKernelFieldHandler(S), Diag(S.getASTContext().getDiagnostics()) {}
Expand All @@ -810,33 +889,17 @@ class SyclKernelFieldChecker
IsInvalid = Diag.Report(FD->getLocation(), diag::err_bad_kernel_param_type)
<< FieldTy;
}

void handleStructType(FieldDecl *FD, QualType FieldTy) final {
if (SemaRef.getASTContext().getLangOpts().SYCLStdLayoutKernelParams &&
!FieldTy->isStandardLayoutType())
IsInvalid =
Diag.Report(FD->getLocation(), diag::err_sycl_non_std_layout_type)
<< FieldTy;
else {
CXXRecordDecl *RD = FieldTy->getAsCXXRecordDecl();
if (!RD->hasTrivialCopyConstructor())

IsInvalid =
Diag.Report(FD->getLocation(),
diag::err_sycl_non_trivially_copy_ctor_dtor_type)
<< 0 << FieldTy;
else if (!RD->hasTrivialDestructor())
IsInvalid =
Diag.Report(FD->getLocation(),
diag::err_sycl_non_trivially_copy_ctor_dtor_type)
<< 1 << FieldTy;
}
IsInvalid = !copyableToKernel(FD, FieldTy);
}

void handleArrayType(const CXXBaseSpecifier &, QualType) final {
// FIXME
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some text here, what do you intend this fixme to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

}

// We should be able to handle this, so we made it part of the visitor, but
// this is 'to be implemented'.
void handleArrayType(FieldDecl *FD, QualType FieldTy) final {
IsInvalid = Diag.Report(FD->getLocation(), diag::err_bad_kernel_param_type)
<< FieldTy;
IsInvalid = !copyableToKernel(FD, FieldTy);
}

void handleOtherType(FieldDecl *FD, QualType FieldTy) final {
Expand Down Expand Up @@ -1437,20 +1500,23 @@ void Sema::ConstructOpenCLKernel(FunctionDecl *KernelCallerFunc,
: CalculatedName);

SyclKernelFieldChecker checker(*this);
SyclKernelDeclCreator kernel_decl(*this, checker, KernelName,
KernelLambda->getLocation(),
KernelCallerFunc->isInlined());
SyclKernelBodyCreator kernel_body(*this, kernel_decl, KernelLambda,
KernelCallerFunc);
SyclKernelIntHeaderCreator int_header(
*this, getSyclIntegrationHeader(), KernelLambda,
calculateKernelNameType(Context, KernelCallerFunc), KernelName,
StableName);

ConstructingOpenCLKernel = true;
VisitRecordFields(KernelLambda->fields(), checker, kernel_decl, kernel_body,
int_header);
ConstructingOpenCLKernel = false;
VisitRecordFields(KernelLambda->fields(), checker);
if (checker.isValid()) {
SyclKernelDeclCreator kernel_decl(*this, checker, KernelName,
KernelLambda->getLocation(),
KernelCallerFunc->isInlined());
SyclKernelBodyCreator kernel_body(*this, kernel_decl, KernelLambda,
KernelCallerFunc);
SyclKernelIntHeaderCreator int_header(
*this, getSyclIntegrationHeader(), KernelLambda,
calculateKernelNameType(Context, KernelCallerFunc), KernelName,
StableName);

ConstructingOpenCLKernel = true;
VisitRecordFields(KernelLambda->fields(), kernel_decl, kernel_body,
int_header);
ConstructingOpenCLKernel = false;
}
}

void Sema::MarkDevice(void) {
Expand Down