Skip to content

[SYCL]Emit an error if an array size exceeds SPIR-V intermediate format limitations #2093

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

Closed
wants to merge 2 commits into from
Closed

[SYCL]Emit an error if an array size exceeds SPIR-V intermediate format limitations #2093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b671be9
[SYCL]Emit an error if array size exceeds 65532 for SPIRV targets
srividya-sundaram Jul 10, 2020
058241b
Remove unwanted braces around 'if'
srividya-sundaram Jul 24, 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
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10908,6 +10908,9 @@ def err_sycl_restrict : Error<
"|use a const static or global variable that is neither zero-initialized "
"nor constant-initialized"
"}0">;
def err_array_num_elements_exceeded
: Error<"Due to SPIR-V intermediate format limitations, constant arrays with number of elements more than 65532 cannot be used in SYCL."
"You can workaround this limitation by splitting the array into several ones">;
Copy link
Contributor

@elizabethandrews elizabethandrews Jul 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not too fond of this diagnostic. How about -

   SPIR-V intermediate format limitation (or can we just say spec here?) disallows arrays with number of 
   elements greater than 65532. You can work around this limitation by splitting the array.

def err_sycl_virtual_types : Error<
"No class with a vtable can be used in a SYCL kernel or any code included in the kernel">;
def note_sycl_recursive_function_declared_here: Note<"function implemented using recursion declared here">;
Expand Down
1 change: 1 addition & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9969,6 +9969,7 @@ class Sema final {
bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);
bool checkAllowedSYCLInitializer(VarDecl *VD,
bool CheckValueDependent = false);
bool isArrayZeroInitialized(VarDecl *VD, bool CheckValueDependent = false);

// Adds an intel_reqd_sub_group_size attribute to a particular declaration.
void addIntelReqdSubGroupSizeAttr(Decl *D, const AttributeCommonInfo &CI,
Expand Down
15 changes: 15 additions & 0 deletions clang/lib/Sema/SemaExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,21 @@ bool Sema::DiagnoseUseOfDecl(NamedDecl *D, ArrayRef<SourceLocation> Locs,
!checkAllowedSYCLInitializer(VD, /*CheckValueDependent =*/true))
SYCLDiagIfDeviceCode(*Locs.begin(), diag::err_sycl_restrict)
<< Sema::KernelConstStaticVariable;
// Emit an error if a const global array's size exceeds SPIR-V
// intermediate format limitations for SPIRV compatible devices.
Comment on lines +232 to +233
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you diagnose only global arrays usage?

else if (IsConst && VD->hasGlobalStorage() &&
Context.getTargetInfo().getTriple().isSPIR() &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking that the target triple is spir-based triple does not guarantee that the target output is SPIR-V. I'm not sure if we use device code output in non-SPIR-V form somewhere, but technically it will be weird if someone will try to emit valid LLVM IR for device code and get an error about something that is not supported in SPIR-V.
However I'm not sure that it is possible to understand that the end target is SPIR-V from FE perspective at all, since SPIR-V translation is happening inside a separate tool on the other compilation stage.
I think possible solution is a way to tell the FE that this code will be converted into SPIR-V, so the driver will do it when SPIR-V emission is required.

!isArrayZeroInitialized(VD, /*CheckValueDependent =*/true)) {
QualType Ty = VD->getType();
if (const auto *CAType = dyn_cast<ConstantArrayType>(Ty)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to use getAsConstantArrayType() here instead of dyn_cast. In another PR, we ran into issues with arrays not being recognized correctly with dyn_cast when it was an elaborated type. @erichkeane can you confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, this could very well be a typedef wrapping the constant array type.

assert(CAType);
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion is weird. If CAType is null, you won't enter this branch at all.

const int MaxNumElements = 65535 - 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line can be removed and you can just check if arrayElements > 65532 below.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should likely be constexpr at least if you're keeping the variable.

int64_t arrayElements = CAType->getSize().getSExtValue();
if (arrayElements > MaxNumElements)
SYCLDiagIfDeviceCode(*Locs.begin(),
diag::err_array_num_elements_exceeded);
}
}
}
}

Expand Down
15 changes: 15 additions & 0 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2077,6 +2077,21 @@ void Sema::finalizeSYCLDelayedAnalysis(const FunctionDecl *Caller,
}
}

bool Sema::isArrayZeroInitialized(VarDecl *VD, bool CheckValueDependent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of CheckValueDependent here? It doesn't seem to be used. Additionally, you likely don't need to use it up the call chain either.

assert(getLangOpts().SYCLIsDevice &&
"Should only be called during device compilation");
Comment on lines +2081 to +2082
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything SYCL-specific in this function. Should we really restrict it for using by other languages/models?

if (VD->isInvalidDecl() || !VD->hasInit() || !VD->hasGlobalStorage())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need !VD->hasGlobalStorage() in here. You already checked it.

return false;
APValue *Value = nullptr;
if (VD->getInit() && !VD->getInit()->isValueDependent())
Value = VD->evaluateValue();
// zero-initialized arrays do not have any initialized elements.
if (Value && Value->isArray() && Value->hasArrayFiller() &&
Copy link
Contributor

@elizabethandrews elizabethandrews Jul 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Are these additional checks required to handle the case int array2[66000] = {}; ? @erichkeane would you know if there is a better way to check this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is overly constrained, yes. If all you care about is whether the array is default initialized (as this seems to be doing?), I believe you can do this without the evaluation. You should be able to just do VD->getAnyInitializer(), which should give you an InitListExpr since this is an array type.

Then, you should be able to loop through its 'inits' to validate whether they evaluate to zero. This would also be able to catch the cases where you get int array[6] = {0,0};, which is ALSO zero initialized, but wouldn't be caught here as far as I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

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

All this starts to become quite complex...
And what if the the array is not used at all and eliminated by some aggressive LLVM passes or if the code is optimized differently or...?
Here we make a lot of guesses ahead on what might happen quite later downstream in code-gen for one specific back-end...

Copy link
Contributor

Choose a reason for hiding this comment

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

Typically we don't suppress warnings based on potential optimizations. If something is illegal with NO optimizations, it also stays illegal with O3. That is, the validity of a program should never depend on the optimization settings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we agree. But here we try to guess in Sema something that is unsupported by some LLVM back-ends... If I understand the title of th PR, it is not about warning but error.

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 think this is overly constrained, yes. If all you care about is whether the array is default initialized (as this seems to be doing?), I believe you can do this without the evaluation. You should be able to just do VD->getAnyInitializer(), which should give you an InitListExpr since this is an array type.

Then, you should be able to loop through its 'inits' to validate whether they evaluate to zero. This would also be able to catch the cases where you get int array[6] = {0,0};, which is ALSO zero initialized, but wouldn't be caught here as far as I can tell.

Are you referring to the if checks as overly constrained here? If yes, then Value->isArray() had to be added because of assert checks in getArrayInitializedElts()

For the case int array[6] = {0,0}; , the first 2 elements will be set to 0 ( and the rest also set to 0) but, Value->getArrayInitializedElts() will be equal to 2 . This will not be a zero-initialized array and error will not be emitted for this case.
I think both the approaches would work, so how is one better than the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is overly constrained, yes. If all you care about is whether the array is default initialized (as this seems to be doing?), I believe you can do this without the evaluation. You should be able to just do VD->getAnyInitializer(), which should give you an InitListExpr since this is an array type.
Then, you should be able to loop through its 'inits' to validate whether they evaluate to zero. This would also be able to catch the cases where you get int array[6] = {0,0};, which is ALSO zero initialized, but wouldn't be caught here as far as I can tell.

Are you referring to the if checks as overly constrained here? If yes, then Value->isArray() had to be added because of assert checks in getArrayInitializedElts()

Except you already know that the type is an array, since it is a ConstantArrayType, right?

For the case int array[6] = {0,0}; , the first 2 elements will be set to 0 ( and the rest also set to 0) but, Value->getArrayInitializedElts() will be equal to 2 . This will not be a zero-initialized array and error will not be emitted for this case.

From my reading of the code, that WILL diagnose (int array[67000] = {0,0}), since it won't be zero initialized, right? Since you'd return false in that case here. That is distinctly the incorrect behavior.

I think both the approaches would work, so how is one better than the other?
Because looping through the inits allows us to better early-escape, evaluating the entire initializer is a REALLY expensive operation, so we can do 'at most' the same amount of work by testing each initializer individually. It also gives us the opportunity to deal with defaulted constructors on record types, which have some otherwise interesting behavior.

Copy link
Contributor Author

@srividya-sundaram srividya-sundaram Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

that WILL diagnose (int array[67000] = {0,0}), since it won't be zero initialized, right

Right, but since the size(6) is < 65532, the error will not be emitted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note the example you quoted. I did a larger array to show that your exception here based on zero-initialized is broken. I was using the '6' simply to be different from the element count (and not 0).

If :
int array[67000] = {};
Shouldn't diagnose, neither should:
int array[67000] = {0,0,{},0};

Nor should:

DefaultConstructible DC[67000] = {};
or
DefaultConstructible DC[67000] = {{}, DefaultConstructible{}};

There is question whether
EmptyDefaultConstructible DC[67000] = {};
or
EmptyDefaultConstructible DC[67000] = {{}, EmptyDefaultConstructible{}};

Should be diagnosed.

Value->getArrayInitializedElts() == 0)
return true;
return false;
}

bool Sema::checkAllowedSYCLInitializer(VarDecl *VD, bool CheckValueDependent) {
assert(getLangOpts().SYCLIsDevice &&
"Should only be called during SYCL compilation");
Expand Down
19 changes: 19 additions & 0 deletions clang/test/SemaSYCL/array-num-elements-exceeded.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// RUN: %clang_cc1 -fsycl -fsycl-is-device -triple spir-unknown -verify -fsyntax-only %s
// RUN: %clang_cc1 -fsycl -fsycl-is-device -triple spir64-unknown -verify -fsyntax-only %s

const double big[67000] = {1.0, 2.0, 3.0};
const int zero_init_array[67000] = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

another good repro is:

const int zero_init_array2[67000] = {0,0,0,constexpr_fun_that_returns_0()};

Copy link
Contributor

Choose a reason for hiding this comment

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

If all you care about is whether the array is default initialized

We care whether zeroinitializer for the array will be generated in LLVM IR. Is there a good method to detect such case on the Sema level?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you'd have to detect the actual initializers. See: https://godbolt.org/z/Yo5bcx

You probably ALSO care about this case: https://godbolt.org/z/v8MKrM

Note that the default constructed "S2" is undef, and I'm not particularly sure what differentiates that. For example, a defaulted constructor on the empty type results in it still being 'undef', but an actual constructor will zero-init and call the constructor at global_var_init: https://godbolt.org/z/fvzK97


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

int main() {
double hostResult = big[0]; // no error thrown, since accessing global const arrays with size > 65532 is valid from host code.
kernel_single_task<class fake_kernel>([]() {
int a = zero_init_array[0]; // no error thrown, since accessing zero-initialized arrays with size > 65532 is valid.
double kernelGlobal = big[0]; // expected-error {{Due to SPIR-V intermediate format limitations, constant arrays with number of elements more than 65532 cannot be used in SYCL.You can workaround this limitation by splitting the array into several ones}}
});
return 0;
}