[SYCL] Add a regression test for the crash during spec constants processing by AlexeySotkin · Pull Request #1536 · intel/llvm

AlexeySotkin · 2020-04-16T18:34:42Z

In case a SYCL kernel captures specialization constants and more than
one accessor it crashes. I think the crash happens due to an extra
increment of KernelFuncParam, in CreateOpenCLKernelBody() method.

Signed-off-by: Alexey Sotkin alexey.sotkin@intel.com

erichkeane

Please add a clang lit test for this that validates what the IR should look like in this case.

This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

erichkeane · 2020-04-16T18:38:39Z

Rewrite here: #1517

bader · 2020-04-16T18:53:29Z

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

AlexeySotkin · 2020-04-17T13:59:39Z

Please add a clang lit test for this that validates what the IR should look like in this case.

This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

AlexeySotkin · 2020-04-17T14:00:22Z

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

Where exactly do you suggest to move the test?

bader · 2020-04-17T14:01:22Z

Do I really need to validate the IR if purpose of this patch is to fix a crash?

No.

bader · 2020-04-17T14:02:12Z

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

Where exactly do you suggest to move the test?

You seems to fix Sema component, so it should be in clang/test/Sema*

erichkeane · 2020-04-17T14:02:50Z

Please add a clang lit test for this that validates what the IR should look like in this case.
This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

Yes, if the problem is that the parameters aren't being passed correctly, which causes the crash. We want our tests fine grained so that they can catch issues like this in the future while still being easy to reproduce.

AlexeySotkin · 2020-04-20T09:10:29Z

Please add a clang lit test for this that validates what the IR should look like in this case.
This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

Yes, if the problem is that the parameters aren't being passed correctly, which causes the crash. We want our tests fine grained so that they can catch issues like this in the future while still being easy to reproduce.

I think that problem is not that the parameters are being passed incorrectly. This PR fixes the crash in the Sema itself (in the same method). Without the fix I've got an out of boundary access at line 703 here

llvm/clang/lib/Sema/SemaSYCL.cpp

Lines 701 to 707 in 6c88351

    
           auto KFP = KernelFuncParam; 
        
           for (size_t I = 0; I < NumParams; ++KFP, ++I) { 
        
             QualType ParamType = (*KFP)->getOriginalType(); 
        
             ParamDREs[I] = DeclRefExpr::Create( 
        
                 S.Context, NestedNameSpecifierLoc(), SourceLocation(), *KFP, 
        
                 false, DeclarationNameInfo(), ParamType, VK_LValue); 
        
           }

AlexeySotkin · 2020-04-22T11:44:28Z

Thank you all for your review. I think I have resolved all your comments with my latest commit.

bader · 2020-04-22T11:46:56Z

Thank you all for your review. I think I have resolved all your comments with my latest commit.

Please, mark resolved comments using "Resolve conversation" button.

bader · 2020-04-22T14:07:13Z

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

erichkeane · 2020-04-22T14:09:21Z

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

Clang used to crash in case SYCL kernel captures a specialization constant before an accessor. The tests checks that the crash is fixed. Signed-off-by: Alexey Sotkin <alexey.sotkin@intel.com>

AlexeySotkin · 2020-04-23T13:02:43Z

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

I have rebased my patch on top of the sycl branch. And now it seems that the test pass without my changes to the Clang.

Honestly, I don't see much sense in this PR anymore

Fznamznon · 2020-04-23T13:05:41Z

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

I have rebased my patch on top of the sycl branch. And now it seems that the test pass without my changes to the Clang.

Honestly, I don't see much sense in this PR anymore

We still need the test.

erichkeane · 2020-04-23T13:06:00Z

I would still like to have the test, if we broke it once, I can see us breaking it again. As I mentioned in my first response, I anticipated that this would be the case.

…_docs * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536)

…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)

Unified clang format

AlexeySotkin requested a review from kbobrovs April 16, 2020 18:34

AlexeySotkin requested review from a team, Fznamznon and erichkeane as code owners April 16, 2020 18:34

AlexeySotkin requested a review from s-kanaev April 16, 2020 18:34

erichkeane reviewed Apr 16, 2020

View reviewed changes

AlexeySotkin force-pushed the private/ansotkin/spec-const-fix branch from e9c8acf to 4878084 Compare April 21, 2020 13:46

bader requested changes Apr 21, 2020

View reviewed changes

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

Fznamznon reviewed Apr 21, 2020

View reviewed changes

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

keryell reviewed Apr 22, 2020

View reviewed changes

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

AlexeySotkin force-pushed the private/ansotkin/spec-const-fix branch from 4878084 to db74bf0 Compare April 22, 2020 11:41

bader reviewed Apr 22, 2020

View reviewed changes

Comment thread clang/test/SemaSYCL/spec_const_accesors.cpp Outdated

AlexeySotkin force-pushed the private/ansotkin/spec-const-fix branch from db74bf0 to adc06b2 Compare April 22, 2020 13:45

erichkeane previously approved these changes Apr 22, 2020

View reviewed changes

AlexeySotkin dismissed erichkeane’s stale review via 3fef4c1 April 22, 2020 13:52

AlexeySotkin force-pushed the private/ansotkin/spec-const-fix branch from adc06b2 to 3fef4c1 Compare April 22, 2020 13:52

bader previously approved these changes Apr 22, 2020

View reviewed changes

bader requested review from Fznamznon and erichkeane April 22, 2020 13:57

erichkeane previously approved these changes Apr 22, 2020

View reviewed changes

Fznamznon previously approved these changes Apr 22, 2020

View reviewed changes

[SYCL] Add test for SYCL kernels with accessor and spec constant

915ac0e

Clang used to crash in case SYCL kernel captures a specialization constant before an accessor. The tests checks that the crash is fixed. Signed-off-by: Alexey Sotkin <alexey.sotkin@intel.com>

AlexeySotkin dismissed stale reviews from Fznamznon, erichkeane, and bader via 915ac0e April 23, 2020 12:59

AlexeySotkin force-pushed the private/ansotkin/spec-const-fix branch from 3fef4c1 to 915ac0e Compare April 23, 2020 12:59

erichkeane approved these changes Apr 23, 2020

View reviewed changes

Fznamznon approved these changes Apr 23, 2020

View reviewed changes

bader changed the title ~~[SYCL] Fix crash in clang for SYCL kernels with accessors and spec constants~~ [SYCL] Add a regression test for the crash during spec constants processing Apr 23, 2020

bader merged commit 7659e45 into sycl Apr 23, 2020

bader deleted the private/ansotkin/spec-const-fix branch April 23, 2020 14:42

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #1536 from ldrumm/unified-clang-format

584bcf0

Unified clang format

Uh oh!

Conversation

AlexeySotkin commented Apr 16, 2020 • edited by s-kanaev Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane commented Apr 16, 2020

Uh oh!

bader commented Apr 16, 2020

Uh oh!

AlexeySotkin commented Apr 17, 2020

Uh oh!

AlexeySotkin commented Apr 17, 2020

Uh oh!

bader commented Apr 17, 2020

Uh oh!

bader commented Apr 17, 2020

Uh oh!

erichkeane commented Apr 17, 2020

Uh oh!

AlexeySotkin commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexeySotkin commented Apr 22, 2020

Uh oh!

Uh oh!

bader commented Apr 22, 2020

Uh oh!

bader commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane commented Apr 22, 2020 • edited by bader Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexeySotkin commented Apr 23, 2020

Uh oh!

Fznamznon commented Apr 23, 2020

Uh oh!

erichkeane commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AlexeySotkin commented Apr 16, 2020 •

edited by s-kanaev

Loading

AlexeySotkin commented Apr 20, 2020 •

edited

Loading

bader commented Apr 22, 2020 •

edited

Loading

erichkeane commented Apr 22, 2020 •

edited by bader

Loading