[SYCLomatic] Create separate util file for groups by abhilash1910 · Pull Request #1784 · oneapi-src/SYCLomatic · GitHub

From email discussion on SYCLcompat for cooperative group apis, this is created to add all header algorithms related to block/warp for sycl.
This PR only replaces the group apis - exchange and sort (from these PRs : #1562 & #1483 ) which were added to the dpct_extensions.h file to group_utils.h file. No other changes apart from removing the existing code from extensions to the group_utils.h file. Correspondingly the tests related to PRs oneapi-src/SYCLomatic-test#601 & oneapi-src/SYCLomatic-test#594 need to be updated accordingly with respect to the changes in namespace only. The aim of this PR is only to remove the functionality from extensions namespace to group_utils namespace.
cc @danhoeflinger @mmichel11 @zhimingwang36 @yihanwg

Is this moving functionality from the other header as is or rewriting + making changes?

If we are moving code and keeping the namespace and everything the same, that is one type of review where we just need to insure that all existing SYCLomatic code can still see the new location, and it is successfully unchanged, etc.
If that is the case we need to remove the other code (where it was moved from).

If we are doing more than just moving code, then we need to review it as "new code", and also figure out how it fits with what is already there.

It would be good to make this clear in the description of the PR (without requiring the context of the email discussion to understand it).

Is this moving functionality from the other header as is or rewriting + making changes?

If we are moving code and keeping the namespace and everything the same, that is one type of review where we just need to insure that all existing SYCLomatic code can still see the new location, and it is successfully unchanged, etc. If that is the case we need to remove the other code (where it was moved from).

If we are doing more than just moving code, then we need to review it as "new code", and also figure out how it fits with what is already there.

It would be good to make this clear in the description of the PR (without requiring the context of the email discussion to understand it).

Yes added description - this is mainly a step for removing the existing sort & exchange coop-group apis from dpct extensions namespace to group_utils namespace without changing any functionality.
This would require an inclusion of dpcpp_extension.h headers for radix_sort because the detail namespace has radix_rank + twiddle_in operations which are essential for sort. As per @yihanwg review, changes in tests no longer needed as the file is included in dpl_utils.h

I'd like to include group_utils.h in dpct/dpl_utils.h, so this PR will not breaking any existing code.

The include guard name should be changed since it is currently mirroring the macro name in the dpcpp_extensions.h file.

It looks like except for the added #include "group_utils.hpp", the other changes are just shifting the order of the includes. Does this matter or can we restore these unneeded changes?

Yes this was caused by the format issue, let me revert to original.

I believe this should be "dpl_extras/functional.h"

Basically LGTM. You should check https://github.com/oneapi-src/SYCLomatic/blob/SYCLomatic/clang/lib/DPCT/HeaderTypes.inc as well to see if it needs to add a new entry.

LGTM, @danhoeflinger @mmichel11 Can you please take another look?

I'm not sure about why, but you may need to add a similar line to this to follow the example of LapackUtilsAllContentStr:

SYCLomatic/clang/lib/DPCT/GenHelperFunction.h

Line 34 in 2233478

extern const std::string LapackUtilsAllContentStr;

(still failing the lit check)

Added , but still fails in lit.

I don't see any issues within group_utils.hpp header, so once the header addition issues are resolved this PR LGTM.

Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>

@yihanwg @zhimingwang36 could you help with the lit failure issue in CI ? Thanks.

@yihanwg @zhimingwang36 could you help with the lit failure issue in CI ? Thanks.

Sure, let me take a look.

[update] Fixed.

Signed-off-by: Wang, Yihan <yihan.wang@intel.com>

LGTM

create separate util file for groups

e61fe3c

abhilash1910 requested a review from a team as a code owner March 13, 2024 10:22

abhilash1910 requested review from AndyCHHuang and tangjj11 March 13, 2024 10:22

abhilash1910 marked this pull request as draft March 13, 2024 10:22

danhoeflinger mentioned this pull request Mar 27, 2024

[SYCLomatic] Block Load headers core #1640

Merged

abhilash1910 and others added 2 commits April 11, 2024 18:45

update

bc314eb

format fix

deb7d56

abhilash1910 marked this pull request as ready for review April 12, 2024 06:12

remove existing code from extensions

30bb358

abhilash1910 mentioned this pull request Apr 14, 2024

[SYCLomatic-test] test load and store headers oneapi-src/SYCLomatic-test#680

Open

yihwang-nv suggested changes Apr 15, 2024

View reviewed changes

add extension headers

99898ee

abhilash1910 commented Apr 15, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

abhilash1910 commented Apr 15, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

include group utils here

4cd9896

abhilash1910 commented Apr 15, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

abhilash1910 and others added 3 commits April 15, 2024 03:58

clang format

007eb9f

format

bc46e70

Update dpl_utils.hpp

90d6d93

mmichel11 reviewed Apr 15, 2024

View reviewed changes

abhilash1910 added 3 commits April 16, 2024 17:01

revert utils.h

a29c786

change mirror header name

13d0a42

Update dpl_utils.hpp

23a0962

mmichel11 reviewed Apr 16, 2024

View reviewed changes

add correct path for functional.h

f71ccb0

abhilash1910 requested a review from yihwang-nv April 16, 2024 16:26

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

1714767

abhilash1910 requested a review from yihwang-nv May 20, 2024 06:17

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

c54a82d

add in header include

0a1c2e3

yihwang-nv approved these changes May 22, 2024

View reviewed changes

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

877ef3e

danhoeflinger reviewed May 22, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

danhoeflinger reviewed May 22, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

Update clang/runtime/dpct-rt/include/dpct/group_utils.hpp

9d187e0

Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>

danhoeflinger reviewed May 22, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

abhilash1910 and others added 3 commits May 22, 2024 20:49

add group utils in helper header

04e91f1

Update clang/runtime/dpct-rt/include/dpct/group_utils.hpp

b0e7b7a

Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>

rm preproc reduction guards

0ea6d1b

danhoeflinger reviewed May 23, 2024

View reviewed changes

Comment thread clang/lib/DPCT/CMakeLists.txt

update cmake

cccccc5

danhoeflinger reviewed May 23, 2024

View reviewed changes

Comment thread clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated

preproc change

5776532

abhilash1910 added 2 commits May 25, 2024 19:35

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

ba14654

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

c7f3594

abhilash1910 and others added 4 commits May 29, 2024 09:22

remove check to test CI

9503fff

force migration

7751c1d

rm process all

cd4627b

[SYCLomatic] Convert group_utils.hpp from CRLF to LF

2ed18b2

Signed-off-by: Wang, Yihan <yihan.wang@intel.com>

yihwang-nv changed the title [SYCLomatic]Create separate util file for groups [SYCLomatic] Create separate util file for groups May 29, 2024

danhoeflinger approved these changes May 29, 2024

View reviewed changes

zhimingwang36 merged commit b2e5588 into oneapi-src:SYCLomatic May 30, 2024

-              #include "dpl_extras/numeric.h"
+              #include "dpl_extras/dpcpp_extensions.h"
               #include "dpl_extras/iterators.h"
+              #include "dpl_extras/memory.h"

+              //
+              //===----------------------------------------------------------------===//
+              #ifndef __DPCT_DPCPP_EXTENSIONS_H__

+              #include "dpct.hpp"
+              #include "dpl_extras/dpcpp_extensions.h"
+              #include "functional.h"

               const std::string LapackUtilsAllContentStr =
               #include "clang/DPCT/lapack_utils.hpp.inc"
                   ;
+              const std::string GroupUtilsAllContentStr =

[SYCLomatic] Create separate util file for groups by abhilash1910 · Pull Request #1784 · oneapi-src/SYCLomatic · GitHub

Uh oh!

[SYCLomatic] Create separate util file for groups - #1784

[SYCLomatic] Create separate util file for groups#1784zhimingwang36 merged 46 commits intooneapi-src:SYCLomaticoneapi-src/SYCLomatic:SYCLomaticfrom abhilash1910:migrate_utils_groupsabhilash1910/SYCLomatic:migrate_utils_groupsCopy head branch name to clipboard

Conversation

abhilash1910 commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danhoeflinger commented Apr 12, 2024

Uh oh!

abhilash1910 commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihwang-nv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmichel11 Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

mmichel11 Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

abhilash1910 Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

mmichel11 Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

yihwang-nv commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihwang-nv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danhoeflinger May 22, 2024

Choose a reason for hiding this comment

Uh oh!

abhilash1910 May 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmichel11 commented May 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abhilash1910 commented May 24, 2024

Uh oh!

yihwang-nv commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danhoeflinger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[SYCLomatic] Create separate util file for groups#1784
zhimingwang36 merged 46 commits into
oneapi-src:SYCLomaticfrom
abhilash1910:migrate_utils_groups

abhilash1910 commented Mar 13, 2024 •
edited
Loading

abhilash1910 commented Apr 14, 2024 •
edited
Loading

yihwang-nv commented May 21, 2024 •
edited
Loading

yihwang-nv left a comment •
edited
Loading

yihwang-nv commented May 28, 2024 •
edited
Loading