Skip to content

[SYCL] Added a new category for device binary properties #3072

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

Merged
merged 3 commits into from
Feb 1, 2021

Conversation

DenisBakhvalov
Copy link
Contributor

@DenisBakhvalov DenisBakhvalov commented Jan 21, 2021

This PR introduces a new category for general properties of device binary images. And it adds an API to retrieve properties from that common category.

smaslov-intel
smaslov-intel previously approved these changes Jan 22, 2021
Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

pi changes look good to me

bader
bader previously approved these changes Jan 22, 2021
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

llvm/* part looks good to me.

@bader bader requested a review from kbobrovs January 23, 2021 09:50
kbobrovs
kbobrovs previously approved these changes Jan 26, 2021
@DenisBakhvalov DenisBakhvalov dismissed stale reviews from kbobrovs, bader, and smaslov-intel via d0939fc January 27, 2021 18:07
romanovvlad
romanovvlad previously approved these changes Jan 27, 2021
@DenisBakhvalov
Copy link
Contributor Author

I think I have all the approvals here. @bader or @kbobrovs, please merge.

@@ -187,6 +187,7 @@ class PropertySetRegistry {
"SYCL/composite specialization constants";
static constexpr char SYCL_DEVICELIB_REQ_MASK[] = "SYCL/devicelib req mask";
static constexpr char SYCL_KERNEL_PARAM_OPT_INFO[] = "SYCL/kernel param opt";
static constexpr char SYCL_IS_ESIMD_IMAGE[] = "SYCL/is ESIMD image";
Copy link
Contributor

Choose a reason for hiding this comment

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

@DenisBakhvalov, as we agreed, can you please change the category name for this to something like SYCL/misc or SYCL/binary image?

@kbobrovs
Copy link
Contributor

I think I have all the approvals here. @bader or @kbobrovs, please merge.

@DenisBakhvalov, please see comment above. If believe the category name change is still pending, right?

@DenisBakhvalov
Copy link
Contributor Author

I think I have all the approvals here. @bader or @kbobrovs, please merge.

@DenisBakhvalov, please see comment above. If believe the category name change is still pending, right?

@kbobrovs Yes, I'm working on it.

@DenisBakhvalov DenisBakhvalov changed the title [SYCL] Added a new device binary property for ESIMD images [SYCL] Added a new category for device binary properties Jan 30, 2021
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

Thanks.

@dendibakh
Copy link
Contributor

@bader @kbobrovs , please merge if no objections.

@bader
Copy link
Contributor

bader commented Feb 1, 2021

image

The approval from @smaslov-intel is required. There were changes in the code he owns since his latest review.

Copy link
Contributor

@smaslov-intel smaslov-intel left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants