Skip to content

[SPIRV] Add convergent attribute to SPIR-V built-ins #1373

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 1 commit into from
Mar 24, 2020
Merged

[SPIRV] Add convergent attribute to SPIR-V built-ins #1373

merged 1 commit into from
Mar 24, 2020

Conversation

bader
Copy link
Contributor

@bader bader commented Mar 23, 2020

The attribute sets SPIR-V built-in semantics for the compiler.

Signed-off-by: Alexey Bader [email protected]

@bader
[SPIRV] Add convergent attribute to SPIR-V built-ins
476430d 
Optimizations can't preserve SPIR-V built-in semantics w/o additional
attribute.

Signed-off-by: Alexey Bader <[email protected]>
@bader bader requested a review from Fznamznon March 23, 2020 09:18
Fznamznon
Fznamznon reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Just wondering, is it possible to test it somehow?
How to make sure that we didn't forget to mark some function?

sycl/include/CL/__spirv/spirv_ops.hpp
@@ -13,6 +13,13 @@
#include <cstdint>
#include <type_traits>

// Convergent attribute
#ifdef __SYCL_DEVICE_ONLY__
#define __SYCL_CONVERGENT__ __attribute__((convergent))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, why SYCL? AFAIK convergent is a non-sycl-specific clang attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clang attribute is not SYCL specific, but macro is.
Do you have better name in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure. I'd probably just named it as CONVERGENT. I also don't see why this macro is SYCL specific. We, for example, have ALWAYS_INLINE macro which somehow is not SYCL specific. So, I'll leave final naming up to you.

@bader
Copy link
Contributor Author

bader commented Mar 23, 2020

Just wondering, is it possible to test it somehow?

This is non-functional change for the current design, so I wasn't able to come-up with a good approach to validate observable behavior change. Let me know if you have any ideas.

How to make sure that we didn't forget to mark some function?

Code review :).

@Fznamznon
Copy link
Contributor

Just wondering, is it possible to test it somehow?

This is non-functional change for the current design, so I wasn't able to come-up with a good approach to validate observable behavior change. Let me know if you have any ideas.

Yep, I also do not have other options except trying to run something with fe optimizations enabled.

How to make sure that we didn't forget to mark some function?

Code review :).

Reviewers also can easy miss something.

@bader
Copy link
Contributor Author

bader commented Mar 24, 2020

This code is supposed to be replaced by #1374, eventually.

@bader bader mentioned this pull request Mar 24, 2020
Merged
@bader bader merged commit 7cca561 into intel:sycl Mar 24, 2020
@bader bader deleted the convergent-attr branch March 24, 2020 15:40
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Mar 27, 2020
@alexbatashev
Merge remote-tracking branch 'upstream/sycl' into private/abatashe/sp…
6790032 
…hinx

* upstream/sycl: (357 commits)
  [Support] Implement a simple tabular data management library (intel#1358)
  [Support] Implement a property set I/O library (intel#1357)
  [SYCL] Fix buffer constructor using iterators (intel#1386)
  [SYCL][FPGA] Enable a set of loop attributes (intel#1312)
  [Driver][SYCL][FPGA] Proper dependency output location when given /Fo<dir> (intel#1346)
  [SPIR-V] Enabling SPIR-V builtin lookup in device SYCL mode (intel#1384)
  [SYCL][NFC] Unify setting kernel arguments (intel#1379)
  [SYCL][Doc] First revision of standard layout relaxation extension (intel#1344)
  [SYCL] Fixed sub-buffer alloca search (intel#1385)
  [SYCL][FPGA] Emit multiple IR variants for the IVDep attribute (intel#1383)
  [SYCL] Add experimental flag to enable front-end optimizations (intel#1376)
  [SYCL] Remove unexpected double in complex SPIR-V for float support (intel#1381)
  [SYCL] Default work-group sizes based on max (intel#952)
  [SYCL][CUDA] Fix usage of multiple backends in the same program (intel#1252)
  [SPIR-V] Add SPIR-V builtin definitions to the builtin lookup.
  [SPIR-V] Add macro definition when -fdeclare-spirv-builtins is activated
  [SYCL] Fix sycl_generic printing
  [SYCL] Support intel::reqd_work_group_size (intel#1328)
  [SYCL][NFC] Make the RT::PiPlugin object private (intel#1375)
  [SPIRV] Add convergent attribute to SPIR-V built-ins (intel#1373)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fznamznon Fznamznon Fznamznon approved these changes

Assignees

@AlexeySotkin AlexeySotkin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bader @Fznamznon @AlexeySotkin