Skip to content

[SYCL] Tie BoundArch to Toolchain #4673

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 4 commits into from
Oct 6, 2021

Conversation

AidanBeltonS
Copy link
Contributor

These changes implement SYCLTargetInfoList which stores both the toolchain and the bound arch. This prevents BoundArchs being incorrectly assigned when actions and dependencies are created. This sets up the possibility of having multiple BoundArchs per triple as a future development.
This fixes an issue where spirv and nvptx swap BoundArch's, causing failure cases. #3631
This patch should also resolve #4662

Some tests were changed in sycl-offload-with-split.c where the output from the device compilation phase was being used as an input by the wrong device. 3 tests are also introduced, two check the ordering behaviour of the triples and the bounded arch. The third checks for behaviour when targetting spirv, nvptx, and amdgcn.

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.

The change looks good conceptually (although @mdtoguchi would have the final say here). The bulk of my comments is style-related - in case any serious concerns arise, I'm hoping to roll all of them out in the next iteration after fully grasping the changes.

@@ -245,7 +245,7 @@
// CHK-PHASE-MULTI-TARG: 3: input, "[[INPUT]]", c++, (device-sycl)
// CHK-PHASE-MULTI-TARG: 4: preprocessor, {3}, c++-cpp-output, (device-sycl)
// CHK-PHASE-MULTI-TARG: 5: compiler, {4}, ir, (device-sycl)
// CHK-PHASE-MULTI-TARG: 6: offload, "host-sycl (x86_64-unknown-linux-gnu)" {2}, "device-sycl (spir64-unknown-unknown)" {5}, c++-cpp-output
// CHK-PHASE-MULTI-TARG: 6: offload, "host-sycl (x86_64-unknown-linux-gnu)" {2}, "device-sycl (spir64_gen-unknown-unknown)" {5}, c++-cpp-output
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self & other reviewers: this change concerned me at first, as I've doubted the deterministic ordering of the target toolchains after the map introduction, but then realized that usage of MapVector for toolchain/arch mapping ensures that.

AGindinson
AGindinson previously approved these changes Oct 2, 2021
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.

Thanks for addressing my comments - LGTM! Approving right away to unblock the CI checks, yet I'd prefer a second approval from the component owners.

@bader bader linked an issue Oct 4, 2021 that may be closed by this pull request
@@ -68,7 +68,7 @@
// RUN: | FileCheck -check-prefix IMPLIED_DEVICE_OBJ %s
// RUN: %clangxx -### -target x86_64-unknown-linux-gnu -fsycl -fintelfpga %S/Inputs/SYCL/objlin64.o %s 2>&1 \
// RUN: | FileCheck -check-prefix IMPLIED_DEVICE_OBJ %s
// IMPLIED_DEVICE_OBJ: clang-offload-bundler{{.*}} "-type=o"{{.*}} "-targets={{.*}}sycl-spir64-unknown-unknown,sycl-spir64_{{.*}}-unknown-unknown"{{.*}} "-unbundle"
// IMPLIED_DEVICE_OBJ: clang-offload-bundler{{.*}} "-type=o"{{.*}} "-targets=sycl-spir64_{{.*}}-unknown-unknown,{{.*}}sycl-spir64-unknown-unknown"{{.*}} "-unbundle"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the ordering swap isn't taking into account the host triple which is coming in at the front of the -targets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I have added the host triple to the start of the expression. It was only failing in windows as if there are device libraries to link with then there is a clang-offload-bundler which does not have the host triple but does have both device triples.

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.

Target triple mismatch [SYCL] Target ordering breaks compilation
4 participants