Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC][SYCL] Move ap_int to sycl::detail namespace #4822

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

MrSidims
Copy link
Contributor

The type isn't used directly by DPCPP programmers.

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

@MrSidims MrSidims requested a review from a team as a code owner October 26, 2021 14:29
@@ -325,280 +325,280 @@ __SYCL_CONVERGENT__ extern SYCL_EXTERNAL void
__spirv_SubgroupBlockWriteINTEL(__attribute__((opencl_global)) uint64_t *Ptr,
dataT Data) noexcept;
template <int W, int rW>
extern SYCL_EXTERNAL ap_int<rW>
__spirv_FixedSqrtINTEL(ap_int<W> a, bool S, int32_t I, int32_t rI,
extern SYCL_EXTERNAL __ap_int<rW>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please do not merge without @tiwaria1 approval

Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing today. Apologies for the delay.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.

@MrSidims
Copy link
Contributor Author

The patch addresses #3374
@keryell FYI

int32_t Quantization = 0, int32_t Overflow = 0) noexcept;
template <int W, int rW>
extern SYCL_EXTERNAL ap_int<rW>
__spirv_FixedRecipINTEL(ap_int<W> a, bool S, int32_t I, int32_t rI,
extern SYCL_EXTERNAL __ap_int<rW>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use sycl::detail::ap_int instead of __ap_int?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear what is required by your toolchain.
But yes, it is better if we can just use plain names in some namespaces rather than top-level names, even if they are somehow hidden with __.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, moved to the namespace.

@MrSidims MrSidims changed the title [NFC][SYCL] Rename ap_int with __ap_int [NFC][SYCL] Move ap_int to sycl::detail namespace Oct 27, 2021
romanovvlad
romanovvlad previously approved these changes Oct 27, 2021
Copy link
Contributor

@romanovvlad romanovvlad left a comment

Choose a reason for hiding this comment

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

LGTM

@dm-vodopyanov
Copy link
Contributor

@MrSidims, can you please address failure in Jenkins/Precommit?

@MrSidims
Copy link
Contributor Author

@MrSidims, can you please address failure in Jenkins/Precommit?

Sure, I'll take a look

The type isn't used directly by DPCPP programmers.

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

@tiwaria1 please take a look

Copy link
Contributor

@keryell keryell left a comment

Choose a reason for hiding this comment

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

OK, so to clarify, now ap_int is in sycl::detail::ap_int and there is no __ap_int, just the sycl::detail::ap_int? If so this looks good.
I am not sure this qualify an [NFC] tag. It breaks the API of anyone using the previous ap_int, so it is a functional change somehow.

Copy link
Contributor

@tiwaria1 tiwaria1 left a comment

Choose a reason for hiding this comment

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

Thanks for making the change Dmitry.

@romanovvlad romanovvlad merged commit d31b42c into intel:sycl Nov 26, 2021
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.

None yet

5 participants