Skip to content
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

[SYCL][Driver] Respect the -target cross-compilation flag. #2417

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

Ralender
Copy link
Contributor

@Ralender Ralender commented Sep 3, 2020

Currently when compiling with for sycl with a -target flag, clang will invoke the device compilation with an auxiliary triple of the machine performing the compilation instead of the host target completely ignoring the -target flag.
This patch fixes this.

bader
bader previously approved these changes Sep 3, 2020
/// Check that -aux-triple is set correctly
// RUN: %clang -### -fsycl -target aarch64-linux-gnu %s 2>&1 \
// RUN: | FileCheck -check-prefix=CHECK-SYCL-AUX-TRIPLE %s
// CHECK-SYCL-AUX-TRIPLE-NOT: clang{{.*}} "x86_64-unknown-linux-gnu"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the value of CHECK-SYCL-AUX-TRIPLE-NOT checks?

I think this test doesn't make sense on aarch64, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this test is trivially true on when running non x86 systems.
there doesn't seem to be a way to query the the triple of the device running the test.
after taking a look at what other cross-compilation test do they don't seem to have check like this so i removed the check.

auto AuxT = llvm::Triple(llvm::sys::getProcessTriple());
llvm::Triple AuxT = C.getDefaultToolChain().getTriple();
if (Args.hasFlag(options::OPT_fsycl_device_only, OptSpecifier(), false))
AuxT = llvm::Triple(llvm::sys::getProcessTriple());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also do this for -fsycl-device-only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When compiling in -fsycl-device-only the default toolchain of the compilation is the device target not the host target. i don't think the host target is actually known when building in -fsycl-device-only.

Copy link
Contributor

@keryell keryell Sep 4, 2020

Choose a reason for hiding this comment

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

@mdtoguchi interesting question... But I have the feeling that compiling for the device is already a kind of cross-compilation per se.
Our use case is when we compile SYCL code on x86 for Xilinx ACAP AIE devices where the host is ARM64 and the devices other specific VLIW DSP.

Copy link
Contributor

Choose a reason for hiding this comment

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

When compiling in -fsycl-device-only the default toolchain of the compilation is the device target not the host target. i don't think the host target is actually known when building in -fsycl-device-only.

It's known (via -aux-triple) and it's required to fix the sizes of data types like int for both: host and device (and for some other reasons). Otherwise host/device ABI might be incompatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-aux-triple is a cc1 only flag so the driver can't really know.
i changed it to be cc1 or dirver.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this might be related to #1814. I'm not sure if we can address Mike's comment easily.
@mdtoguchi, should we resolve it in scope of #1814 or there is an easy fix here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use of -fsycl-device-only should behave in a simlar fashion to -fsycl in regards to the device compilation step. The disconnects need to be addressed as stated in #1814 . The behavior being seen with the error emitted when using -fsycl-targets with -fsycl -fsycl-device-only is not intentional.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mdtoguchi, let me check that I understand current status correctly.
Am I right that handling -fsycl-device-only+-target together is blocked by #1814 and we agree to resolve it separately?

Anyway, I think we all agree that we should not add -aux-triple driver option. Right?

Copy link
Contributor Author

@Ralender Ralender Sep 9, 2020

Choose a reason for hiding this comment

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

@mdtoguchi, let me check that I understand current status correctly.
Am I right that handling -fsycl-device-only+-target together is blocked by #1814 and we agree to resolve it separately?

I brought back the patch before a driver -aux-triple to resolve it separately.

Anyway, I think we all agree that we should not add -aux-triple driver option. Right?

I agree and I think the behavior you described is a better solution than a driver -aux-triple.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. #1814 should be resolved separately and we do not need -aux-triple driver option.

@Ralender Ralender force-pushed the FixCrossCompile branch 2 times, most recently from 8fccbf9 to f09eec9 Compare September 7, 2020 10:03
@bader
Copy link
Contributor

bader commented Sep 7, 2020

@Ralender, could you resolve the conflicts with the tip of the sycl branch, please?

@Ralender
Copy link
Contributor Author

Ralender commented Sep 7, 2020

fixed

@bader bader requested a review from mdtoguchi September 10, 2020 15:13
@bader
Copy link
Contributor

bader commented Sep 10, 2020

@Ralender, @mdtoguchi, thanks!

@bader bader merged commit 2aab337 into intel:sycl Sep 10, 2020
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.

4 participants