Skip to content

[Driver][SYCL] Update default SPIR device arch to correlate with host #4936

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 2 commits into from
Nov 15, 2021

Conversation

mdtoguchi
Copy link
Contributor

The default target arch for the device compilation when using -fsycl
should match the target arch of the host compilation.

x86_64 (64-bit) -> spir64
i686 (32-bit) -> spir

Adjust locations where this is set (offload and device-only compiles)
and modify the corresponding tests.

The default target arch for the device compilation when using -fsycl
should match the target arch of the host compilation.

x86_64 (64-bit) -> spir64
i686 (32-bit)   -> spir

Adjust locations where this is set (offload and device-only compiles)
and modify the corresponding tests.
@mdtoguchi mdtoguchi marked this pull request as ready for review November 10, 2021 20:43
Comment on lines +1028 to +1032
if (HasValidSYCLRuntime) {
StringRef SYCLTargetArch = getDefaultSYCLArch(C);
if (SYCLfpga)
// Triple for -fintelfpga is spir64_fpga.
SYCLTargetArch = "spir64_fpga";
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: this does impact full compilation scenarios, right? In this case, does the E2E testing coverage that we have prove that E2E compilation/execution of DPC++ apps it's going to work on 32-bit systems? Or instead, could it be that it has never worked properly due to pointer size mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In all honestly, I don't think we have done any testing on 32-bit systems. All of our configurations are 64-bit hosted and the use of spir64 is commonplace. The issue of the mismatch was only found due to the single internal 32-bit Windows build configuration (which isn't tested, it is just a build AFAIK)

@AGindinson AGindinson requested review from kbobrovs and bader November 12, 2021 08:38
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.

LGTM if no concerns from other reviewers

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