[SYCLomatic] Create separate util file for groups #1784

abhilash1910 · 2024-03-13T10:22:30Z

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

danhoeflinger · 2024-04-12T13:20:21Z

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).

abhilash1910 · 2024-04-14T06:07:41Z

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

yihanwg

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

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

mmichel11 · 2024-04-15T18:04:48Z

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

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


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

mmichel11 · 2024-04-15T18:08:02Z

clang/runtime/dpct-rt/include/dpct/dpl_utils.hpp

 #include "dpl_extras/iterators.h"
+#include "dpl_extras/memory.h"


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.

mmichel11 · 2024-04-16T15:00:08Z

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

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


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

yihanwg · 2024-05-21T02:40:15Z

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.

yihanwg

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

danhoeflinger · 2024-05-22T13:42:33Z

clang/lib/DPCT/GenHelperFunction.cpp

@@ -73,6 +73,9 @@ const std::string FftUtilsAllContentStr =
 const std::string LapackUtilsAllContentStr =
 #include "clang/DPCT/lapack_utils.hpp.inc"
    ;
+const std::string GroupUtilsAllContentStr =


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.

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

mmichel11 · 2024-05-22T14:33:26Z

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

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

Co-authored-by: Dan Hoeflinger <[email protected]>

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

Co-authored-by: Dan Hoeflinger <[email protected]>

clang/lib/DPCT/CMakeLists.txt

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

abhilash1910 · 2024-05-24T11:27:45Z

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

yihanwg · 2024-05-28T06:39:03Z

@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 <[email protected]>

danhoeflinger

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 tangjj11 and AndyCHHuang 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

yihanwg suggested changes Apr 15, 2024

View reviewed changes

add extension headers

99898ee

abhilash1910 commented Apr 15, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

abhilash1910 commented Apr 15, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

include group utils here

4cd9896

abhilash1910 commented Apr 15, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

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 yihanwg April 16, 2024 16:26

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

1714767

abhilash1910 requested a review from yihanwg May 20, 2024 06:17

Merge branch 'oneapi-src:SYCLomatic' into migrate_utils_groups

c54a82d

add in header include

0a1c2e3

yihanwg 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

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

danhoeflinger reviewed May 22, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

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

9d187e0

Co-authored-by: Dan Hoeflinger <[email protected]>

danhoeflinger reviewed May 22, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

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 <[email protected]>

rm preproc reduction guards

0ea6d1b

danhoeflinger reviewed May 23, 2024

View reviewed changes

clang/lib/DPCT/CMakeLists.txt Show resolved Hide resolved

update cmake

cccccc5

danhoeflinger reviewed May 23, 2024

View reviewed changes

clang/runtime/dpct-rt/include/dpct/group_utils.hpp Outdated Show resolved Hide resolved

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 <[email protected]>

yihanwg 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
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCLomatic] Create separate util file for groups #1784

[SYCLomatic] Create separate util file for groups #1784

abhilash1910 commented Mar 13, 2024 •

edited

Loading

danhoeflinger commented Apr 12, 2024

abhilash1910 commented Apr 14, 2024 •

edited

Loading

yihanwg left a comment

mmichel11 Apr 15, 2024

mmichel11 Apr 15, 2024

abhilash1910 Apr 16, 2024

mmichel11 Apr 16, 2024

yihanwg commented May 21, 2024 •

edited

Loading

yihanwg left a comment •

edited

Loading

danhoeflinger May 22, 2024

abhilash1910 May 23, 2024

mmichel11 commented May 22, 2024

abhilash1910 commented May 24, 2024

yihanwg commented May 28, 2024 •

edited

Loading

danhoeflinger left a comment

		#include "dpl_extras/iterators.h"
		#include "dpl_extras/memory.h"

[SYCLomatic] Create separate util file for groups #1784

[SYCLomatic] Create separate util file for groups #1784

Conversation

abhilash1910 commented Mar 13, 2024 • edited Loading

danhoeflinger commented Apr 12, 2024

abhilash1910 commented Apr 14, 2024 • edited Loading

yihanwg left a comment

Choose a reason for hiding this comment

mmichel11 Apr 15, 2024

Choose a reason for hiding this comment

mmichel11 Apr 15, 2024

Choose a reason for hiding this comment

abhilash1910 Apr 16, 2024

Choose a reason for hiding this comment

mmichel11 Apr 16, 2024

Choose a reason for hiding this comment

yihanwg commented May 21, 2024 • edited Loading

yihanwg left a comment • edited Loading

Choose a reason for hiding this comment

danhoeflinger May 22, 2024

Choose a reason for hiding this comment

abhilash1910 May 23, 2024

Choose a reason for hiding this comment

mmichel11 commented May 22, 2024

abhilash1910 commented May 24, 2024

yihanwg commented May 28, 2024 • edited Loading

danhoeflinger left a comment

Choose a reason for hiding this comment

abhilash1910 commented Mar 13, 2024 •

edited

Loading

abhilash1910 commented Apr 14, 2024 •

edited

Loading

yihanwg commented May 21, 2024 •

edited

Loading

yihanwg left a comment •

edited

Loading

yihanwg commented May 28, 2024 •

edited

Loading