Skip to content

Conversation

@VyacheslavLevytskyy
Copy link
Contributor

@VyacheslavLevytskyy VyacheslavLevytskyy commented Jan 13, 2025

We introduce here SPIR-V Backend support in LIT config by adding a new LIT feature 'spirv-backend', and in a way that substitutions in end-to-end tests are aware about the new LIT feature.

@VyacheslavLevytskyy
Copy link
Contributor Author

It was agreed that we split #16317 into three parts:

Comment on lines 191 to 192
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main problem is our choice for the %{sycl_triple} substitution. If it included the entire option and not its value only, we could have extended that, and it would make the most sense. IIUC, spirv-backend option is only needed when we use -fsycl-targets=spir*[,.*]*. As such, fixing that might be the right way to implement this PR.

@sarnex , @steffenlarsen , @AlexeySachkov , WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexeySachkov , WDYT?

Do we have a test which needs %{sycl_triple} for something other than -fsycl-targets? If not, then I fully support replacing it with something like %{fsycl-targets} which will simplify passing extra arguments which are unique to specific targets

Copy link
Contributor

Choose a reason for hiding this comment

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

// RUN: %clangxx -fsycl -fsycl-targets=%{sycl_triple},spir64 -o %t1.out %s
, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that its a good idea to have a higher-level substitution, even if some tests won't be using it (I assume that there are just a small fraction of tests which pass triple explicitly because they want to test something specific).

And we can decide on a case-by-case basis if any of those remaining tests needs to be specialized to run with SPIR-V backend.

I also wouldn't block the PR on this refactoring: considering that passing -fsycl-use-spirv-backend-for-spirv-gen won't do anything for non-SPIRV targets the refactoring seems to be unrelated to this and the code doesn't look that bad to me in its current form.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry is the idea here to make sycl_triple be -fsycl-targets=whatever,whatever instead of whatever,whatever and if this feature is enabled we just append the option to sycl_triple? if so sounds good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

If it "won't do anything", just pass it inside %clangxx. That would make it working for tests that don't use %{build}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic of the change is that %{build} means now two different things, either "build with support of SPIR-V Backend" or "build", whereas previously it meant just "build". It's implemented by modifying %{build} contents depending on the use case.

I has no personal preference as for the way of how to implement this. We need to pass -fsycl-use-spirv-backend-for-spirv-gen parameter as the part of %{build} when "spirv-backend" is in test.config.available_features, that's all.

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic of the change is that %{build} means now two different things

I'm not sure what you mean, but it doesn't sound right to me.

We need to pass -fsycl-use-spirv-backend-for-spirv-gen parameter as the part of %{build} when "spirv-backend" is in test.config.available_features, that's all.

I don't agree with this either. IMO, we need to pass the option anytime we're compiling for spirv triple, and %{build} is definitely the wrong place to do that.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

i dont have strong enough of an opinion to hold up the PR on the sycl_targets thing, but probably andrei should state his opinion before merging

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It's possible to use the LLVM SPIR-V Backend instead of llvm-spirv tool to
It's possible to use the LLVM SPIR-V Backend instead of the `llvm-spirv` tool to

Co-authored-by: aelovikov-intel <[email protected]>
@VyacheslavLevytskyy
Copy link
Contributor Author

I rebased the PR to #16614 and aligned with refactoring of the %{sycl_target_opts} substitution.

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@steffenlarsen steffenlarsen merged commit 2add39f into intel:sycl Jan 16, 2025
17 checks passed
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.

5 participants