Skip to content

[NFC][SYCL] Add pragma unroll in accessor.hpp#4347

Merged
bader merged 11 commits intointel:syclfrom
MrSidims:private/MrSidims/Unroll
Aug 24, 2021
Merged

[NFC][SYCL] Add pragma unroll in accessor.hpp#4347
bader merged 11 commits intointel:syclfrom
MrSidims:private/MrSidims/Unroll

Conversation

@MrSidims
Copy link
Contributor

It shall improve performance on various targets in case if we
have 3 dimensional calculations within nested loops.

Signed-off-by: Dmitry Sidorov dmitry.sidorov@intel.com

It shall improve performance on various targets in case if we
have 3 dimensional calculations within nested loops.

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims requested a review from a team as a code owner August 16, 2021 11:41
@MrSidims MrSidims requested a review from rbegam August 16, 2021 11:41
@MrSidims
Copy link
Contributor Author

/summary:run

1 similar comment
@MrSidims
Copy link
Contributor Author

/summary:run

@MrSidims
Copy link
Contributor Author

@s-kanaev please take a look

Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Other than the questions the patch looks good.

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
erichkeane
erichkeane previously approved these changes Aug 18, 2021
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

I'm more in favor of this approach, I think it at least is less of a 'big hammer'.

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

Should there be a test for it?

@MrSidims
Copy link
Contributor Author

Hah, build for cuda is still failing with:
accessor.hpp:841:0: error: ignoring #pragma GCC unroll [-Werror=unknown-pragmas]
#pragma GCC unroll 3

that's a bit unexpected, I'll check tomorrow why it happens.

Should there be a test for it?

@s-kanaev I can barely imagine a proper test for this patch. One option would be to add a test to check_device_code and see unroll metadata appearing on the appropriate branch instruction, but I guess clang itself has lots of tests for that. Also it doesn't include host code compilation on various compilers. Another option would be to, hm, dump assembly? But I can barely imagine, how we can do it (for example) for MSVC compilation in LIT environment. Or there is another option that you have in mind?

Dmitry Sidorov added 2 commits August 19, 2021 11:04
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
s-kanaev
s-kanaev previously approved these changes Aug 19, 2021
Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims requested a review from s-kanaev August 21, 2021 14:29
@MrSidims
Copy link
Contributor Author

/summary:run

@MrSidims MrSidims requested a review from AaronBallman August 23, 2021 18:55
AaronBallman
AaronBallman previously approved these changes Aug 24, 2021
Copy link
Contributor

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@s-kanaev s-kanaev left a comment

Choose a reason for hiding this comment

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

LGTM

@bader
Copy link
Contributor

bader commented Aug 24, 2021

LGTM

@s-kanaev, please, approve using GitHub UI to unblock the merge.

s-kanaev
s-kanaev previously approved these changes Aug 24, 2021
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims dismissed stale reviews from s-kanaev and AaronBallman via e9d4e64 August 24, 2021 14:22
@MrSidims MrSidims force-pushed the private/MrSidims/Unroll branch from 9559942 to e9d4e64 Compare August 24, 2021 14:22
Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>
@MrSidims MrSidims requested a review from bader August 24, 2021 14:36
@bader
Copy link
Contributor

bader commented Aug 24, 2021

@MrSidims, I think, it's more likely that the failure on Linux machine was introduced by @alexbatashev with 739487c rather than this patch.
@alexbatashev, could you take a look, please?

@romanovvlad, FYI.

@alexbatashev
Copy link
Contributor

@bader this sneaked in before my patch was merged but after it passed CI. Working on a fix.

@bader bader merged commit a10199d into intel:sycl Aug 24, 2021
#else
#define __SYCL_UNROLL(x)
#endif // compiler switch
#endif // __SYCL_UNROLL
Copy link
Contributor

Choose a reason for hiding this comment

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

While looking at this I wonder why just using a metaprogrammed loop instead is not better than this pragmatic spaghetti plate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also works with one-iteration loops, unlike this pragma-based solution: #6560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants