Skip to content

[SYCL] no more ast visitor usage for variable type checks. #1513

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 5 commits into from
Apr 15, 2020
Merged

[SYCL] no more ast visitor usage for variable type checks. #1513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62b088a
moved VLA test, updated lit tests
cperkinsintel Apr 13, 2020
0a78215
removing CheckSYCLType and calls to it. Several AST Visitor methods r…
cperkinsintel Apr 13, 2020
5681620
clang-formattery
cperkinsintel Apr 13, 2020
cba85e5
added some markers so the crazy nested notes get tidied up, at least …
cperkinsintel Apr 14, 2020
75c6efd
clang formatation
cperkinsintel Apr 14, 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
102 changes: 4 additions & 98 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,10 @@ static void checkSYCLVarType(Sema &S, QualType Ty, SourceRange Loc,
emitDeferredDiagnosticAndNote(S, Loc, diag::err_typecheck_zero_array_size,
UsedAtLoc);

// variable length arrays
if (Ty->isVariableArrayType())
emitDeferredDiagnosticAndNote(S, Loc, diag::err_vla_unsupported, UsedAtLoc);

// Sub-reference array or pointer, then proceed with that type.
while (Ty->isAnyPointerType() || Ty->isArrayType())
Ty = QualType{Ty->getPointeeOrArrayElementType(), 0};
Expand Down Expand Up @@ -284,9 +288,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
: RecursiveASTVisitor<MarkDeviceFunction>(), SemaRef(S) {}

bool VisitCallExpr(CallExpr *e) {
for (const auto &Arg : e->arguments())
CheckSYCLType(Arg->getType(), Arg->getSourceRange());

if (FunctionDecl *Callee = e->getDirectCallee()) {
Callee = Callee->getCanonicalDecl();
assert(Callee && "Device function canonical decl must be available");
Expand All @@ -308,8 +309,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
SemaRef.Diag(e->getExprLoc(), diag::err_sycl_restrict)
<< Sema::KernelCallVirtualFunction;

CheckSYCLType(Callee->getReturnType(), Callee->getSourceRange());

if (auto const *FD = dyn_cast<FunctionDecl>(Callee)) {
// FIXME: We need check all target specified attributes for error if
// that function with attribute can not be called from sycl kernel. The
Expand Down Expand Up @@ -338,12 +337,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
return true;
}

bool VisitCXXConstructExpr(CXXConstructExpr *E) {
for (const auto &Arg : E->arguments())
CheckSYCLType(Arg->getType(), Arg->getSourceRange());
return true;
}

bool VisitCXXTypeidExpr(CXXTypeidExpr *E) {
SemaRef.Diag(E->getExprLoc(), diag::err_sycl_restrict) << Sema::KernelRTTI;
return true;
Expand All @@ -354,35 +347,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
return true;
}

bool VisitTypedefNameDecl(TypedefNameDecl *TD) {
CheckSYCLType(TD->getUnderlyingType(), TD->getLocation());
return true;
}

bool VisitRecordDecl(RecordDecl *RD) {
CheckSYCLType(QualType{RD->getTypeForDecl(), 0}, RD->getLocation());
return true;
}

bool VisitParmVarDecl(VarDecl *VD) {
CheckSYCLType(VD->getType(), VD->getLocation());
return true;
}

bool VisitVarDecl(VarDecl *VD) {
CheckSYCLType(VD->getType(), VD->getLocation());
return true;
}

bool VisitDeclRefExpr(DeclRefExpr *E) {
Decl *D = E->getDecl();
if (SemaRef.isKnownGoodSYCLDecl(D))
return true;

CheckSYCLType(E->getType(), E->getSourceRange());
return true;
}

// The call graph for this translation unit.
CallGraph SYCLCG;
// The set of functions called by a kernel function.
Expand Down Expand Up @@ -506,64 +470,6 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
}

private:
bool CheckSYCLType(QualType Ty, SourceRange Loc) {
llvm::DenseSet<QualType> visited;
return CheckSYCLType(Ty, Loc, visited);
}

bool CheckSYCLType(QualType Ty, SourceRange Loc,
llvm::DenseSet<QualType> &Visited) {
if (Ty->isVariableArrayType()) {
SemaRef.Diag(Loc.getBegin(), diag::err_vla_unsupported);
return false;
}

while (Ty->isAnyPointerType() || Ty->isArrayType())
Ty = QualType{Ty->getPointeeOrArrayElementType(), 0};

// Pointers complicate recursion. Add this type to Visited.
// If already there, bail out.
if (!Visited.insert(Ty).second)
return true;

if (const auto *ATy = dyn_cast<AttributedType>(Ty))
return CheckSYCLType(ATy->getModifiedType(), Loc, Visited);

if (const auto *CRD = Ty->getAsCXXRecordDecl()) {
// If the class is a forward declaration - skip it, because otherwise we
// would query property of class with no definition, which results in
// clang crash.
if (!CRD->hasDefinition())
return true;

for (const auto &Field : CRD->fields()) {
if (!CheckSYCLType(Field->getType(), Field->getSourceRange(),
Visited)) {
if (SemaRef.getLangOpts().SYCLIsDevice)
SemaRef.Diag(Loc.getBegin(), diag::note_sycl_used_here);
return false;
}
}
} else if (const auto *RD = Ty->getAsRecordDecl()) {
for (const auto &Field : RD->fields()) {
if (!CheckSYCLType(Field->getType(), Field->getSourceRange(),
Visited)) {
if (SemaRef.getLangOpts().SYCLIsDevice)
SemaRef.Diag(Loc.getBegin(), diag::note_sycl_used_here);
return false;
}
}
} else if (const auto *FPTy = dyn_cast<FunctionProtoType>(Ty)) {
for (const auto &ParamTy : FPTy->param_types())
if (!CheckSYCLType(ParamTy, Loc, Visited))
return false;
return CheckSYCLType(FPTy->getReturnType(), Loc, Visited);
} else if (const auto *FTy = dyn_cast<FunctionType>(Ty)) {
return CheckSYCLType(FTy->getReturnType(), Loc, Visited);
}
return true;
}

Sema &SemaRef;
};

Expand Down
25 changes: 17 additions & 8 deletions clang/test/SemaSYCL/sycl-restrict.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,16 @@ void no_restriction(int p) {
int index[p + 2];
}
void restriction(int p) {
int index[p + 2]; // expected-error {{variable length arrays are not supported for the current target}}
// This particular violation is nested under two kernels with intermediate function calls.
// e.g. main -> 1stkernel -> usage -> 2ndkernel -> isa_B -> restriction -> !!
// Because the error is in two different kernels, we are given helpful notes for the origination of the error, twice.
// expected-note@#call_usage {{called by 'operator()'}}
// expected-note@#call_kernelFunc {{called by 'kernel_single_task<fake_kernel, (lambda at}}
// expected-note@#call_isa_B 2{{called by 'operator()'}}
// expected-note@#call_rtti_kernel {{called by 'usage'}}
// expected-note@#rtti_kernel 2{{called by 'kernel1<kernel_name, (lambda at }}
// expected-note@#call_vla 2{{called by 'isa_B'}}
int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}}
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the two errors here? How'd we get the extra one? I'd like to understand how this happens.

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 have a theory .

Everything else in the isa_B function context has double the number of expectation values than you would expect. I think this is because the calling hierarchy looks like this:
main -> kernel -> usage -> kernel -> isa_B -> restriction
Note that there is a kernel inside the usage function, which is called from a different kernel.

The change in this PR moves the diagnostics for VLA to be a Deferred Diagnostic, like most of the others. The Deferred Diagnostics walk the final produced kernel looking to see if there were any warnings deferred earlier for that thing, and then they emit.

So my theory is the the old system, using the AST Visitor, only visited it once (and then issued an immediate diagnostic). But with this change ,the deferred diagnostic system essentially visits it twice, because of the nested kernels.

I did a quick test and move that call to restriction() up a level to usage() , and now it fires just once, not twice.

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 can make that change to the sycl-restrict, to make it clearer. I do not know why there are nested kernels for these tests. Let me know if you would like a change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I just want to make sure its still an appropriate amount. sycl-restrict is criminally oversized unfortunately, so understanding this stuff is annoying. If there is a simplification that you can makes it clearer, I would be appreciative.

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 there any reason for having nested kernels in these tests? Or is it just an artifact of this files history?

I'd be happy to get rid of the kernel nesting which would tidy things up a bit. But I don't want to undo anything important.

Otherwise, I can simply move the testing of the VLA . My own inclination is to get rid of the nested kernels entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry to be dense. Like this?

Suggested change
int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}}
// expected-note@71 2{{called by 'isa_B'}}
int index[p + 2]; // expected-error 2{{variable length arrays are not supported for the current target}}

Is there another way of using the "@" syntax so it's not absolute line numbers? I have used the relative "@+1" to put these immediately before a line, but in the case we are discussing, the caller we are mentioning is far away. This approach seems like it'll just break the minute someone edits the file.
Is this -verify expectation system documented anywhere? I've searched, but the llvm-lit docs don't talk about this part. Are there other ways of using the "@" in the expectation?

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. A veritable rosetta stone, that. Do you have the decipher key for Linear A 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.

Negative ghostrider.

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, I have pushed added markers and moved the notes into reasonable places for the VLA test, which is the one test this PR touches.

I would like to just go ahead and finish that and move all the stray note expectations in sycl-restrict.cpp and put them close to their caller. Is that ok to do in this PR? Or should I open a different one for that?

}
} // namespace Check_VLA_Restriction

Expand Down Expand Up @@ -67,8 +76,8 @@ bool isa_B(A *a) {
if (f1 == f2) // expected-note 2{{called by 'isa_B'}}
return false;

Check_VLA_Restriction::restriction(7);
int *ip = new int; // expected-error 2{{SYCL kernel cannot allocate storage}}
Check_VLA_Restriction::restriction(7); //#call_vla
int *ip = new int; // expected-error 2{{SYCL kernel cannot allocate storage}}
int i;
int *p3 = new (&i) int; // no error on placement new
OverloadedNewDelete *x = new (struct OverloadedNewDelete); // expected-note 2{{called by 'isa_B'}}
Expand All @@ -79,7 +88,7 @@ bool isa_B(A *a) {

template <typename N, typename L>
__attribute__((sycl_kernel)) void kernel1(L l) {
l(); // expected-note 6{{called by 'kernel1<kernel_name, (lambda at }}
l(); //#rtti_kernel // expected-note 6{{called by 'kernel1<kernel_name, (lambda at }}
}
} // namespace Check_RTTI_Restriction

Expand Down Expand Up @@ -189,9 +198,9 @@ void usage(myFuncDef functionPtr) {
// expected-error@+1 {{SYCL kernel cannot use a non-const global variable}}
b.f(); // expected-error {{SYCL kernel cannot call a virtual function}}

Check_RTTI_Restriction::kernel1<class kernel_name>([]() { // expected-note 3{{called by 'usage'}}
Check_RTTI_Restriction::kernel1<class kernel_name>([]() { //#call_rtti_kernel // expected-note 3{{called by 'usage'}}
Check_RTTI_Restriction::A *a;
Check_RTTI_Restriction::isa_B(a); // expected-note 6{{called by 'operator()'}}
Check_RTTI_Restriction::isa_B(a); //#call_isa_B // expected-note 6{{called by 'operator()'}}
});

// ======= Float128 Not Allowed in Kernel ==========
Expand Down Expand Up @@ -323,7 +332,7 @@ int use2(a_type ab, a_type *abp) {

template <typename name, typename Func>
__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
kernelFunc(); // expected-note 7{{called by 'kernel_single_task<fake_kernel, (lambda at}}
kernelFunc(); //#call_kernelFunc // expected-note 7{{called by 'kernel_single_task<fake_kernel, (lambda at}}
}

int main() {
Expand All @@ -340,7 +349,7 @@ int main() {
auto notACrime = &commitInfraction;

kernel_single_task<class fake_kernel>([=]() {
usage(&addInt); // expected-note 5{{called by 'operator()'}}
usage(&addInt); //#call_usage // expected-note 5{{called by 'operator()'}}
a_type *p;
use2(ab, p); // expected-note 2{{called by 'operator()'}}
});
Expand Down