Skip to content

[Driver][SYCL] Improve tool name specification for AOT tools on Windows #2389

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.

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

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

mdtoguchi
Copy link
Contributor

For Windows, we need to be sure to search for the full executable name
as opposed to just the case name for the AOT tools (aoc, ocloc, opencl-aot).
There can be instances of directory names of the same name, causing the
wrong 'binary' to be called.

@mdtoguchi mdtoguchi requested a review from AGindinson as a code owner August 31, 2020 00:49
Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

Could Driver::GetProgramPath() be adjusted instead? This would probably scale better, and would involve non-AOT tools as well (or is there a reason why such a case can't fire for the latter?)

@mdtoguchi
Copy link
Contributor Author

mdtoguchi commented Aug 31, 2020

Could Driver::GetProgramPath() be adjusted instead? This would probably scale better, and would involve non-AOT tools as well (or is there a reason why such a case can't fire for the latter?)

The issue I see with modifying GetProgramPath is that the user may not want .exe added to their file (maybe there is no .exe variant available on Windows).

Edit: I may be able to simplify in GetProgramPath instead and make sure that whatever is being searched for isn't a directory.

@mdtoguchi
Copy link
Contributor Author

@AGindinson, I took a look, and the behaviors cannot be simply modified at the driver level. I would like to keep the changes as is.

@mdtoguchi
Copy link
Contributor Author

/summary:run

@mdtoguchi mdtoguchi requested a review from AGindinson September 2, 2020 01:47
AGindinson
AGindinson previously approved these changes Sep 2, 2020
Copy link
Contributor

@AGindinson AGindinson left a comment

Choose a reason for hiding this comment

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

I've tried finding something in llvm::sys::fs/::path that would give a full path to the executable and/or extract its actual extension, but failed miserably :)
Also, the Linux failure in a SYCL RT test doesn't seem related to the patch.

For Windows, we need to be sure to search for the full executable name
as opposed to just the case name for the AOT tools (aoc, ocloc, opencl-aot).
There can be instances of directory names of the same name, causing the
wrong 'binary' to be called.
@bader bader merged commit 78a86da into intel:sycl Sep 3, 2020
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
[L0] Set exec info for all L0 kernels in UR kernel
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.

3 participants