Skip to content

[SYCL] Add extension and implement fp control kernel property #11591

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Nov 16, 2023
Merged

[SYCL] Add extension and implement fp control kernel property #11591

merged 21 commits into from
Nov 16, 2023

Conversation

againull
Copy link
Contributor

No description provided.

@againull againull temporarily deployed to WindowsCILock October 19, 2023 00:45 — with GitHub Actions Inactive
@againull againull temporarily deployed to WindowsCILock October 19, 2023 01:26 — with GitHub Actions Inactive
@againull againull force-pushed the fp_control_via_kernel_level_attr branch from 9bbbe0b to b9ea81c Compare October 31, 2023 16:55
@againull againull temporarily deployed to WindowsCILock October 31, 2023 16:57 — with GitHub Actions Inactive
@againull againull force-pushed the fp_control_via_kernel_level_attr branch 4 times, most recently from f96167d to 58d52ae Compare November 2, 2023 22:10
@againull againull temporarily deployed to WindowsCILock November 2, 2023 22:12 — with GitHub Actions Inactive
@againull againull force-pushed the fp_control_via_kernel_level_attr branch from 58d52ae to 238db10 Compare November 3, 2023 08:01
@againull againull marked this pull request as ready for review November 3, 2023 08:03
@againull againull requested review from a team as code owners November 3, 2023 08:03
gmlueck
gmlueck reviewed Nov 3, 2023
View reviewed changes
Pennycook
Pennycook reviewed Nov 3, 2023
View reviewed changes
Pennycook
Pennycook reviewed Nov 3, 2023
View reviewed changes
v-klochkov
v-klochkov reviewed Nov 3, 2023
View reviewed changes
@againull
Remove unnecessary alt/ieee mode control
5da85a4 
These modes turn out to be needed only for legacy directx 9.
@againull againull requested a review from a team as a code owner November 15, 2023 09:26
@againull againull requested a review from gmlueck November 15, 2023 09:38
gmlueck
gmlueck reviewed Nov 15, 2023
View reviewed changes
elizabethandrews
elizabethandrews reviewed Nov 15, 2023
View reviewed changes
clang/lib/CodeGen/CodeGenFunction.cpp Outdated
@@ -1176,8 +1176,13 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
A->getFilteredAttributeNameValuePairs(CGM.getContext());

llvm::AttrBuilder FnAttrBuilder(Fn->getContext());
for (const auto &NameValuePair : NameValuePairs)
for (const auto &NameValuePair : NameValuePairs) {
if (NameValuePair.first == "sycl-floating-point-control" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't emit diagnostics in CodeGen. I'm actually not sure if/when community finds CodeGen diagnostic acceptable. Maybe @AaronBallman will have insights.

Having said that, we don't introspect properties currently in Sema (and we should aim to keep it that way to ensure compile time properties don't just become another version of attributes). So I don't think this diagnostic can be implemented in the FE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, reverted frontend changes.
Changed this to static assert in SYCL headers - error is emitted if introduced compile time property is applied on non-ESIMD kernel. One downside of this is that error will not be generated if to compile with -fsycl-device-only, i.e. error is generated if to compile with -fsycl (this is because Kernel::isESIMD() is specialized in integration header and this specialization is available only for host compilation). I wasn't able to overcome that and didn't find another way which will not look ugly.
@gmlueck Would such static assert be an acceptable approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's OK.

@againull againull requested a review from gmlueck November 15, 2023 18:37
@againull
Fix lit tests
2fcad9f 
Update checks because masks changed + other fixes
@againull
Copy link
Contributor Author

@AlexeySachkov Could you please take a look?

@againull againull requested a review from v-klochkov November 16, 2023 18:07
@aelovikov-intel aelovikov-intel merged commit bf8ea96 into intel:sycl Nov 16, 2023
@againull againull deleted the fp_control_via_kernel_level_attr branch December 22, 2023 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elizabethandrews elizabethandrews elizabethandrews left review comments

@gmlueck gmlueck gmlueck approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@v-klochkov v-klochkov v-klochkov approved these changes

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@Pennycook Pennycook Awaiting requested review from Pennycook

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@againull @Pennycook @gmlueck @AlexeySachkov @v-klochkov @elizabethandrews @steffenlarsen @aelovikov-intel