Skip to content

[SYCL] Reject zero length arrays #1153

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.

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 19 commits into from
Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
6 changes: 6 additions & 0 deletions clang/lib/Sema/SemaType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2340,6 +2340,12 @@ QualType Sema::BuildArrayType(QualType T, ArrayType::ArraySizeModifier ASM,
<< ArraySize->getSourceRange();
ASM = ArrayType::Normal;
}

// Zero length arrays are disallowed in SYCL device code.
if (getLangOpts().SYCLIsDevice)
SYCLDiagIfDeviceCode(ArraySize->getBeginLoc(),
diag::err_typecheck_zero_array_size)
<< ArraySize->getSourceRange();
} else if (!T->isDependentType() && !T->isVariablyModifiedType() &&
!T->isIncompleteType() && !T->isUndeducedType()) {
// Is the array too large?
Expand Down
104 changes: 104 additions & 0 deletions clang/test/SemaSYCL/deferred-diagnostics-emit.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// RUN: %clang_cc1 -fsycl -triple spir64 -fsycl-is-device -verify -fsyntax-only %s
//
// Ensure that the SYCL diagnostics that are typically deferred, correctly emitted.

// testing that the deferred diagnostics work in conjunction with the SYCL namespaces.
inline namespace cl {
namespace sycl {
class queue {
public:
template <typename T>
void submit(T CGF) {}
};

template <int I>
class id {};

template <int I>
class range {};
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the purpose of defining these classes, it is unnecessary and makes test fat. Can we just use one function with sycl_kernel attribute? Only this is needed to show the compiler where the device code starts from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fznamznon
We cannot just throw the sycl_kernel attribute on a function like we do in some of the other tests - what we are looking for, specifically, is that the deferred diagnostics are working with namespaces.
Remember, there is that overhaul of the deferred diagnostic system which was merged and then backed out - that overhaul passed all existing tests but yet caused all of our SYCL deferred diagnostics to suddenly not work. The purpose of this deferred-diagnostics-emit.cpp is to provide defensive testing that will fail if that overhaul makes the same mistake and is later re-introduced. This code you are identifying came from the case where we isolated the problem when used against that overhaul. It has been confirmed against that. Before I can change it, I'll have to reconfirm.

This PR has been open for over a week now, and is really quite small. This particular test file is defensive. I would prefer that we merge it because we all have other things demanding our attention. I can look into trimming this side test file, if you insist, but my preference is to merge this PR rather than having it languish another week. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you want to capture some problems and we won't forget about them, I appreciate it. But all that I'm asking is reduce the test case to have minimal reproducer for our problems. You want capture that we have problems with namespaces - use namespaces, instead of adding not necessary stuff. All these classes will appear in AST dumps for module and will make debugging harder.
BTW. We have tons of ugly/bad/unnecessary changes in SYCL just because someone was running out of time. So the fact that it's "long conversation"/"we don't have time", "someone very needed to merge it yesterday" etc, won't force me to give approve for the patch if this patch doesn't fix crash which happens in the compiler on every second code sample.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


class handler {
public:
template <typename KernelName, typename KernelType, int Dims>
__attribute__((sycl_kernel)) void kernel_parallel_for(KernelType kernelFunc) {
// expected-note@+1 2{{called by 'kernel_parallel_for<AName, (lambda}}
kernelFunc(id<1>{});
}
template <typename KernelName, typename KernelType, int Dims>
void parallel_for(range<Dims> NWI, KernelType kernelFunc) {
kernel_parallel_for<KernelName, KernelType, Dims>(kernelFunc);
}
};
} // namespace sycl
} // namespace cl

//variadic functions from SYCL kernels emit a deferred diagnostic
void variadic(int, ...);

int calledFromKernel(int a) {
// expected-error@+1 {{zero-length arrays are not permitted in C++}}
int MalArray[0];

// expected-error@+1 {{__float128 is not supported on this target}}
__float128 malFloat = 40;

//expected-error@+1 {{SYCL kernel cannot call a variadic function}}
variadic(5);

return a + 20;
}

// template used to specialize a function that contains a lambda that should
// result in a deferred diagnostic being emitted.
// HOWEVER, this is not working presently.
// TODO: re-test after new deferred diagnostic system is merged.
// restore the "FIX!!" tests below

template <typename T>
void setup_sycl_operation(const T VA[]) {

cl::sycl::range<1> numOfItems;
cl::sycl::queue deviceQueue;

deviceQueue.submit([&](cl::sycl::handler &cgh) {
cgh.parallel_for<class AName>(numOfItems, [=](cl::sycl::id<1> wiID) {
// FIX!! xpected-error@+1 {{zero-length arrays are not permitted in C++}}
int OverlookedBadArray[0];

// FIX!! xpected-error@+1 {{__float128 is not supported on this target}}
__float128 overlookedBadFloat = 40;
});
});
}

int main(int argc, char **argv) {

// --- direct lambda testing ---
cl::sycl::range<1> numOfItems;
cl::sycl::queue deviceQueue;

deviceQueue.submit([&](cl::sycl::handler &cgh) {
cgh.parallel_for<class AName>(numOfItems, [=](cl::sycl::id<1> wiID) {
// expected-error@+1 {{zero-length arrays are not permitted in C++}}
int BadArray[0];

// expected-error@+1 {{__float128 is not supported on this target}}
__float128 badFloat = 40; // this SHOULD trigger a diagnostic

//expected-error@+1 {{SYCL kernel cannot call a variadic function}}
variadic(5);

// expected-note@+1 {{called by 'operator()'}}
calledFromKernel(10);
});
});

// --- lambda in specialized function testing ---

//array A is only used to feed the template
const int array_size = 4;
int A[array_size] = {1, 2, 3, 4};
setup_sycl_operation(A);

return 0;
}
3 changes: 3 additions & 0 deletions clang/test/SemaSYCL/sycl-restrict.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ void usage(myFuncDef functionPtr) {

// expected-error@+1 {{__float128 is not supported on this target}}
__float128 A;

// expected-error@+1 {{zero-length arrays are not permitted in C++}}
int BadArray[0];
}

namespace ns {
Expand Down