[SYCL] Update image accessor constructor #5312

IgorKharchikov · 2022-01-14T18:38:04Z

Changed image_accessor() constructor from "= default" to user defined to make it possible to use const references in parallel_for.

Redefinition of default constructor image_accessor() (previously used "= default") to make it possible to use const references in parallel_for.

sycl/test/check_device_code/const_ref_image_accessor.cpp

Reduced ad hoc test file.

Fixed typo

Rewrite of RUN part.

Created new test for fixed issue with mismatched namespace for ctz function.

[SYCL]Created regression test for ctz fix

…ed_tests Revert "[SYCL]Created regression test for ctz fix"

sycl/test/check_device_code/const_ref_image_accessor.cpp

Reduced test file so it contains only func() function, edited commented line for test running.

sycl/test/check_device_code/const_ref_image_accessor.cpp

Edited commented line for testing, corrected typos in code.

bader

Thanks. LGTM.

IgorKharchikov · 2022-01-18T15:00:43Z

It appears, that new test always fails, I'm starting to investigate the problem.

bader · 2022-01-18T15:04:56Z

Cool. It seems we found an ICE in the SYCL front-end compiler. @intel/dpcpp-cfe-reviewers, could you take a look, please?

IgorKharchikov · 2022-01-18T15:33:59Z

Problem appears to be connected with SYCL_EXTERNAL, I'm currently in process of familiarizing with theory around it's appliance.

bader · 2022-01-18T15:39:23Z

Problem appears to be connected with SYCL_EXTERNAL, I'm currently in process of familiarizing with theory around it's appliance.

I don't see any problems with using SYCL_EXTERNAL here. Anyway the compiler should not crash, but give an error message if SYCL_EXTERNAL usage is incorrect.

premanandrao · 2022-01-20T18:15:39Z

Problem appears to be connected with SYCL_EXTERNAL, I'm currently in process of familiarizing with theory around it's appliance.

I don't see any problems with using SYCL_EXTERNAL here. Anyway the compiler should not crash, but give an error message if SYCL_EXTERNAL usage is incorrect.

I believe @Fznamznon just fixed this with PR #5342.

bader · 2022-01-20T18:19:35Z

@premanandrao, thanks for letting us know.
@IgorKharchikov, please, update your branch to check if this problem is fixed.

IgorKharchikov · 2022-01-20T19:01:07Z

@bader, thanks, a merge was performed, but the problem still exists, though error messages are new. I'm starting to investigate them.

Removed SYCL_EXTERNAL attribute due to it's unnecessity in this test (parallel_for is used here).

bader · 2022-01-25T08:23:22Z

@bader, thanks, a merge was performed, but the problem still exists, though error messages are new. I'm starting to investigate them.

The test still uses "heavy" parallel_for. What is the status of the problem with using SYCL_EXTERNAL? Has it been resolved?

IgorKharchikov · 2022-01-25T08:31:49Z

@bader, parallel_for was mentioned in the ticket and an ad hoc reproducer, so I decided to add it for visibility of test, as for SYCL_EXTERNAL, it appeared to be redundant here and it was causing conflicts with parallel_for.

bader · 2022-01-25T13:15:49Z

@bader, parallel_for was mentioned in the ticket and an ad hoc reproducer, so I decided to add it for visibility of test, as for SYCL_EXTERNAL, it appeared to be redundant here and it was causing conflicts with parallel_for.

Is parallel_for required to reproduce the issue? It doesn't look like a source of the problem. Right?

IgorKharchikov · 2022-01-25T14:16:42Z

@bader, in fact, parallel_for was not causing any issues, I understand your demand, the change was applied in the last commit, thanks for help in understanding of writing a succinct test.

sycl/test/check_device_code/const_ref_image_accessor.cpp

Co-authored-by: Alexey Bader <[email protected]>

IgorKharchikov added 2 commits January 14, 2022 21:20

Updated image_accessor constructor

225c6c0

Redefinition of default constructor image_accessor() (previously used "= default") to make it possible to use const references in parallel_for.

Added test for image_accessor const ref fix

667ce93

IgorKharchikov requested a review from a team as a code owner January 14, 2022 18:38

IgorKharchikov requested a review from s-kanaev January 14, 2022 18:38

bader reviewed Jan 14, 2022

View reviewed changes

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

IgorKharchikov added 7 commits January 17, 2022 19:09

Update const_ref_image_accessor.cpp

0645ee6

Reduced ad hoc test file.

Update const_ref_image_accessor.cpp

a6eefc8

Fixed typo

Update const_ref_image_accessor.cpp

f5641da

Rewrite of RUN part.

[SYCL]Created regression test for ctz fix

b8f7509

Created new test for fixed issue with mismatched namespace for ctz function.

Merge pull request #1 from IgorKharchikov/IgorKharchikov-added_tests

d433b34

[SYCL]Created regression test for ctz fix

Revert "[SYCL]Created regression test for ctz fix"

0f54611

Merge pull request #2 from IgorKharchikov/revert-1-IgorKharchikov-add…

42ef2fa

…ed_tests Revert "[SYCL]Created regression test for ctz fix"

IgorKharchikov requested a review from bader January 18, 2022 13:10

bader reviewed Jan 18, 2022

View reviewed changes

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

Update const_ref_image_accessor.cpp

7465d36

Reduced test file so it contains only func() function, edited commented line for test running.

bader requested changes Jan 18, 2022

View reviewed changes

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

bader reviewed Jan 18, 2022

View reviewed changes

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

Update const_ref_image_accessor.cpp

9b4be53

Edited commented line for testing, corrected typos in code.

bader previously approved these changes Jan 18, 2022

View reviewed changes

Merge branch 'intel:sycl' into sycl

bcf96d3

IgorKharchikov added 2 commits January 24, 2022 14:32

Merge branch 'intel:sycl' into sycl

f30e0b4

Update const_ref_image_accessor.cpp

333ea45

Removed SYCL_EXTERNAL attribute due to it's unnecessity in this test (parallel_for is used here).

IgorKharchikov dismissed bader’s stale review via 333ea45 January 24, 2022 11:36

vladimirlaz previously approved these changes Jan 25, 2022

View reviewed changes

vladimirlaz requested a review from bader January 25, 2022 08:07

bader changed the title ~~[SYCL] Updated image accessor constructor~~ [SYCL] Update image accessor constructor Jan 25, 2022

Update const_ref_image_accessor.cpp

e650676

IgorKharchikov dismissed vladimirlaz’s stale review via e650676 January 25, 2022 14:13

bader reviewed Jan 25, 2022

View reviewed changes

sycl/test/check_device_code/const_ref_image_accessor.cpp Outdated Show resolved Hide resolved

Update sycl/test/check_device_code/const_ref_image_accessor.cpp

92a181c

Co-authored-by: Alexey Bader <[email protected]>

bader requested review from bader and vladimirlaz January 25, 2022 17:56

vladimirlaz approved these changes Jan 26, 2022

View reviewed changes

bader merged commit 3070b95 into intel:sycl Jan 26, 2022

[SYCL] Update image accessor constructor #5312

[SYCL] Update image accessor constructor #5312

Uh oh!

Conversation

IgorKharchikov commented Jan 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

IgorKharchikov commented Jan 18, 2022

Uh oh!

bader commented Jan 18, 2022

Uh oh!

IgorKharchikov commented Jan 18, 2022

Uh oh!

bader commented Jan 18, 2022

Uh oh!

premanandrao commented Jan 20, 2022

Uh oh!

bader commented Jan 20, 2022

Uh oh!

IgorKharchikov commented Jan 20, 2022

Uh oh!

bader commented Jan 25, 2022

Uh oh!

IgorKharchikov commented Jan 25, 2022

Uh oh!

bader commented Jan 25, 2022

Uh oh!

IgorKharchikov commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!