Skip to content

[SYCL] Fix spriv-to-ir-wrapper invocation in clang-linker-wrapper #14247

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 11 commits into from
Jul 3, 2024
Merged

[SYCL] Fix spriv-to-ir-wrapper invocation in clang-linker-wrapper #14247

merged 11 commits into from
Jul 3, 2024

Conversation

maksimsab
Copy link
Contributor

@maksimsab maksimsab commented Jun 20, 2024 •
edited
Loading

Fix command line argument passed to "spirv-to-ir-wrapper" invocation in clang-linker-wrapper.
Also make clang-linker-wrapper print arguments quoted if needed.

@maksimsab
Copy link
Contributor Author

maksimsab commented Jun 21, 2024 •
edited
Loading

Also this change fixes the following E2E tests in the New Offloading Model.

SYCL :: Basic/multisource_spv_obj.cpp​
SYCL :: Basic/spirv_device_obj_smoke.cpp​

@maksimsab maksimsab marked this pull request as ready for review June 21, 2024 13:18
@maksimsab maksimsab requested review from a team as code owners June 21, 2024 13:18
sarnex
sarnex approved these changes Jun 21, 2024
View reviewed changes
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.

CmdArgs part LGTM, I don't understand the printing part so I'll leave that to others.

@maksimsab
Copy link
Contributor Author

The point of my change is that we pass the string with whitespaces like "--spirv-option1 --spirv-option2" as one command line argument. The issue before my change was that a printed command was unquoted and it differs from the perspective of the shell. More precisely, it starts being interpreted as two command line arguments instead of one.
My change adds quotation when it is needed.

@maksimsab maksimsab requested a review from asudarsa June 27, 2024 14:28
asudarsa
asudarsa approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

asudarsa
asudarsa reviewed Jun 27, 2024
View reviewed changes
@asudarsa asudarsa self-requested a review June 27, 2024 17:25
@maksimsab
Copy link
Contributor Author

Update: Removed printArg usage. I will have to get it back more accurate directly in llvm-project.

asudarsa
asudarsa approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks

@maksimsab
Copy link
Contributor Author

@intel/llvm-gatekeepers Can we merge it? Failure in e2e Windows Intel GEN12 is irrelevant.

@sarnex
Copy link
Contributor

sarnex commented Jul 3, 2024 •
edited
Loading

Usually I would ask we rerun win gen12 testing but since I know the code changed has no e2e test coverage anyway, i'll just merge it now

@sarnex sarnex merged commit 3dda8b4 into intel:sycl Jul 3, 2024
13 of 14 checks passed
@maksimsab maksimsab deleted the fix_fsycl_device_obj branch July 3, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarnex sarnex sarnex approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@asudarsa asudarsa asudarsa approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@maksimsab @sarnex @mdtoguchi @asudarsa