8000 [SYCL][ESIMD] Add driver option to enable SYCL Explicit SIMD extension. by kbobrovs · Pull Request #1743 · intel/llvm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[SYCL][ESIMD] Add driver option to enable SYCL Explicit SIMD extension. #1743

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 2 commits into from
Jun 3, 2020
Merged

[SYCL][ESIMD] Add driver option to enable SYCL Explicit SIMD extension. #1743

merged 2 commits into from
Jun 3, 2020

Conversation

kbobrovs
Copy link
Contributor

Options name: -fsycl-explicit-simd.
This option is temporary until ESIMD and normal SYCL kernels can
co-exist in the same source.

Signed-off-by: Konstantin S Bobrovsky konstantin.s.bobrovsky@intel.com

@kbobrovs kbobrovs requested a review from erichkeane May 25, 2020 07:18
@kbobrovs
[SYCL][ESIMD] Add driver option to enable SYCL Explicit SIMD extension.
96b0af3 
Options name: -fsycl-explicit-simd.
This option is temporary until ESIMD and normal SYCL kernels can
co-exist in the same source.

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>
@keryell
Copy link
Contributor
keryell commented May 26, 2020

Is it explained somewhere what is ESIMD?

@erichkeane
Copy link
Contributor

This patch needs to come with a LanguageExtensions.rst entry that explains what this is/does. Please add that first.

@kbobrovs
Copy link
Contributor Author

Is it explained somewhere what is ESIMD?

Yes. #1731

@kbobrovs
Copy link
Contributor Author

This patch needs to come with a LanguageExtensions.rst entry that explains what this is/does. Please add that first.

This is the spec of the extension, same documentation style as all other extensions
#1731

@bader
Copy link
Contributor
bader commented May 28, 2020 •
edited
Loading

Options name: -fsycl-explicit-simd.
This option is temporary until ESIMD and normal SYCL kernels can
co-exist in the same source.

How this option helps "ESIMD and normal SYCL kernels to co-exist in the same source"?
Why not just emit an error (or warning) instead of adding a new option?
NOTE: diagnostics can be controlled with existing options e.g. disabled or converted from a warning to an error.

@kbobrovs
Copy link
Contributor Author

Options name: -fsycl-explicit-simd.
This option is temporary until ESIMD and normal SYCL kernels can
co-exist in the same source.

How this option helps "ESIMD and normal SYCL kernels to co-exist in the same source"?
Why not just emit an error (or warning) instead of adding a new option?
NOTE: diagnostics can be controlled with existing options e.g. disabled or converted from a warning to an error.

It does not help co-existence. What it does is
(1) makes compiler set the SYCL_EXPLICIT_SIMD macro, which is then used in headers in few places to change behavior
(2) makes FE change behavior in some cases - e.g. to allow non-constant globals

@kbobrovs
Copy link
Contributor Author

See more details in #1731

@bader
Copy link
Contributor
bader commented May 28, 2020

Options name: -fsycl-explicit-simd.
This option is temporary until ESIMD and normal SYCL kernels can
co-exist in the same source.

How this option helps "ESIMD and normal SYCL kernels to co-exist in the same source"?
Why not just emit an error (or warning) instead of adding a new option?
NOTE: diagnostics can be controlled with existing options e.g. disabled or converted from a warning to an error.

It does not help co-existence. What it does is
(1) makes compiler set the SYCL_EXPLICIT_SIMD macro, which is then used in headers in few places to change behavior
(2) makes FE change behavior in some cases - e.g. to allow non-constant globals

Sorry, but I have a few more questions:

(1) Why this macro should be enabled by the compiler rather than developer or in some library header? Why this macro name does not start with the underscore, which is reserved by the C++ spec for the compiler needs? Where can I find the information about the behavior it's supposed to change?
(2) Do you have a list of FE changes requiring a new driver option? Allowing non-constant globals make sense in regular SYCL mode as well. OpenCL 2.+ environment supports it. Can we have a separate option to relax this restriction or just lift it for post-SYCL-1.2.1? Will it be enough?

See more details in #1731

I can't find anything related to new driver option, macro or FE changed behavior in this document. Should it be updated?

Is it related somehow to implementation restrictions?

@kbobrovs
Copy link
Contributor Author
kbobrovs commented May 28, 2020 •
edited
Loading

(1) Why this macro should be enabled by the compiler rather than developer or in some library header?

This is similar to SYCL_EXTERNAL - I think there was earlier discussion that it is safer to enable in the compiler. But I think it can be defined in library header - should not be a problem. But in this case user would have to be very careful about header inclusion order, and make sure defining header is always included even if it is not needed otherwise. Do you think it is better? Why?

Why this macro name does not start with the underscore, which is reserved by the C++ spec for the compiler needs?

This is github swallowing the __. It is __SYCL_EXPLICIT_SIMD__

Where can I find the information about the behavior it's supposed to change?

Do you mean __SYCL_EXPLICIT_SIMD__ macro? This will be in the runtime patch. For now you can access our dpcpp-cm gitlab repo and grep the runtime sources - let me know if you need a link.

(2) Do you have a list of FE changes requiring a new driver option? Allowing non-constant globals make sense in regular SYCL mode as well. OpenCL 2.+ environment supports it. Can we have a separate option to relax this restriction or just lift it for post-SYCL-1.2.1? Will it be enough?

LLVM opts:

  • --disable-llvm-passes is not added
  • 3 new ESIMD-specific LLVM passes added
  • A number of existing LLVM passes added after FE and before SPIRV generation

FE:

  • Alignment attribute on return value is not added to improve code gen. @DenisBakhvalov may comment more on this one
  • Non-const globals are allowed
  • Zero initialization for non-const globals is skipped
  • (Not implemented yet) Some of the semantics checks identifying unsupported cases. This is TBD.

But, as I said in #1731, eventually we'd like to remove that option. So can we go a slightly different way here - start with -fsycl-explicit-simd, which isolates the rest of SYCL from ESIMD-related modification., then incrementally eliminate the differences between ESIMD and normal SYCL. Once they are all eliminated, we safely get rid of (deprecate) the -fsycl-explicit-simd.
?

See more details in #1731

I can't find anything related to new driver option, macro or FE changed behavior in this document. Should it be updated?

I was actually referring to the PR description, not to the doc. The doc is a spec. I think we should have a different doc describing ESIMD compiler usage and temporary restrictions.

Is it related somehow to implementation restrictions?

Indirectly - via the __SYCL_EXPLICIT_SIMD__ macro changing runtime behavior in some cases. Directly - enforcing extra semantics checks in the FE (see notes above)

@kbobrovs kbobrovs mentioned this pull request May 28, 2020
Merged
@kbobrovs
Copy link
Contributor Author

@bader, please comment

Fznamznon
Fznamznon previously approved these changes Jun 2, 2020
View reviewed changes
@kbobrovs
Review: removed TODO
e5ee9f8 
Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>
erichkeane
erichkeane approved these changes Jun 3, 2020
View reviewed changes
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.

Looks good for the CFE, @mdtoguchi needs to approve the driver parts.

mdtoguchi
mdtoguchi approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor
@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

Driver changes LGTM.

@pvchupin pvchupin merged commit 88e56d9 into intel:sycl Jun 3, 2020
@kbobrovs kbobrovs deleted the esimd-driver branch July 30, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@erichkeane erichkeane erichkeane approved these changes

@Fznamznon Fznamznon Fznamznon left review comments

@AGindinson AGindinson Awaiting requested review from AGindinson

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

Successfully merging this pull request may close these issues.
7 participants
@kbobrovs @keryell @erichkeane @bader @Fznamznon @mdtoguchi @pvchupin
0