[SYCL] Rework the way we handle SYCL -cc1 arguments. #3259

AaronBallman · 2021-02-24T17:28:38Z

-fsycl-is-device and -fsycl-is-host are mutually exclusive options that
now imply -fsycl mode. So passing -fsycl-is-device will now set both
the SYCL and SYCLIsDevice language options.

This introduces driver errors if the user attempts to pass both host
and device mode on the command line. Further, it disallows you from
specifying the -sycl-std= option unless in device or host mode.

There is some testing fallout from these changes because some tests
were passing -fsycl-is-device and not passing -fsycl which meant that
SYCL mode was never enabled for those tests.

-fsycl-is-device and -fsycl-is-host are mutually exclusive options that now imply -fsycl mode. So passing -fsycl-is-device will now set both the SYCL and SYCLIsDevice language options. This introduces driver errors if the user attempts to pass both host and device mode on the command line. Further, it disallows you from specifying the -sycl-std= option unless in device or host mode. There is some testing fallout from these changes because some tests were passing -fsycl-is-device and not passing -fsycl which meant that SYCL mode was never enabled for those tests.

AaronBallman · 2021-02-24T17:31:07Z

I wanted to get some early feedback on the approach I'm taking, but I believe this work should actually be landed upstream and then pulled in from there.

I believe there is one failing test case for clang/CodeGenSYCL/loop_unroll.cpp that I will likely need some help figuring out. The test was not actually testing in SYCL mode and so it was likely an invalid test to begin with.

erichkeane

1 comment that likely requires driver work, but I don't want fsycl to work in cc1 at all. I think it should mean "do the 3 invocations thing" to the driver, and nothing to the cc1 invocation.

clang/lib/Frontend/CompilerInvocation.cpp

The *vast* majority of the changes are to to the tests.

clang/lib/Frontend/CompilerInvocation.cpp

clang/include/clang/Driver/Options.td

clang/test/CodeGenSYCL/loop_unroll.cpp

clang/test/SemaSYCL/intel-fpga-mem-builtin.cpp

clang/test/SemaSYCL/intel-fpga-reg.cpp

premanandrao · 2021-02-24T21:01:30Z

I believe there is one failing test case for clang/CodeGenSYCL/loop_unroll.cpp that I will likely need some help figuring out. The test was not actually testing in SYCL mode and so it was likely an invalid test to begin with.

My quick thought is that you should modify the test (which was invalid to begin with) to accept the new behavior with your change.

Adds back -fno-sycl as a noop. Removes the diagnostic about passing -sycl-std= but not -fsycl-is-device or -fsycl-is-host. Fixes several tests, but not all of them yet.

If -sycl-std is passed to the driver without -fsycl, then it is not passed to the -cc1 invocation.

It doesn't make sense to pass -fsycl -Xclang -fsycl-is-host because that makes an invalid device compilation pass that has -fsycl-is-host and -fsycl-is-device at the same time.

AaronBallman · 2021-02-25T21:41:39Z

Now that it looks like I've gotten everything to pass for us, I think the next steps are to figure out how to land this. I think these changes need to go upstream, so my intention is to spin up a community review for these changes. However, once this lands here through a pulldown, we'll have a lot of broken tests. My plan is to leave this PR languish as a Draft until pulldown breaks things, then resurrect it to fix the test failures. If anyone has suggestions on how to make it less painful for us, I'm all ears.

AaronBallman · 2021-02-26T12:39:18Z

Two things:

I have no idea what's going on with the Jenkins failures. The log files manage to contain both too much and too little information at the same time. Where do these tests live in the source tree? From the logs, I see Cloning repository https://github.com/otcshare/llvm_ci.git and yet... I can't seem to clone that repo.
When I started doing the upstreaming work, I found out that community has SYCL and SYCLIsDevice, but not SYCLIsHost. That makes upstreaming this... awkward, to say the least. I'm not certain how to proceed on the community side of things.

bader · 2021-03-09T13:34:57Z

@intel/llvm-reviewers-runtime, ping.

bader · 2021-03-09T13:38:03Z

@intel/llvm-reviewers-runtime, ping.

There is only one line of changes for you to review: sycl/test/check_device_code/id_queries_fit_int.cpp. :)

bader · 2021-03-09T13:42:39Z

sycl/test/check_device_code/id_queries_fit_int.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsycl -Xclang -fsycl-is-host -fsycl-id-queries-fit-in-int -O0 -c -S -emit-llvm -o %t.ll %s
+// RUN: %clangxx -fsycl -fsycl-id-queries-fit-in-int -O0 -c -S -emit-llvm -o %t.ll %s


It looks like this test is supposed to check LLVM IR emitted by the host compiler. Right?

Should we remove -fsycl as well? Is there a better way to get the result of host-side compilation?

(not relevant to this PR) Should it be located in sycl/test/check_device_code/ directory, if it doesn't check device code at all?

It was unclear what this test was trying to accomplish with that RUN line. -fsycl kicks off a device and a host build, so it wasn't obvious what was intended when running in host-and-device-at-the-same-time mode (which isn't supported).

If the test is only interested in host mode functionality, it should remove -fsycl and pass -Xclang -fsycl-is-host.

If the test is only interested in host mode functionality, it should remove -fsycl and pass -Xclang -fsycl-is-host.

I think so. There is no device code in the test.

It looks like this test is supposed to check LLVM IR emitted by the host compiler. Right?

Exactly.

By the way, what's the purpose of this file change?

I agree. The test already had this flow, so it's better to address it separately.

It seems that passing the path was insufficient to correct the test. Given that the test is already in a broken state, I'd prefer if someone with more domain knowledge handled correcting the test deficiencies in a separate PR (so I would back to the original changes to the test, which were originally passing).

To validate what @s-kanaev says is intended we need to re-write the code to make it device code instead.

Then, there must be an issue for us to not forget about it.

@s-kanaev -- I filed #3335 to track this.

Great thanks!

AaronBallman requested review from AGindinson, elizabethandrews, mdtoguchi and premanandrao as code owners February 24, 2021 17:28

AaronBallman marked this pull request as draft February 24, 2021 17:29

AaronBallman requested a review from erichkeane February 24, 2021 17:29

erichkeane reviewed Feb 24, 2021

View reviewed changes

clang/lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

AGindinson requested a review from bader February 24, 2021 18:12

No longer allow -cc1 -fsycl at all.

2004883

The *vast* majority of the changes are to to the tests.

erichkeane reviewed Feb 24, 2021

View reviewed changes

clang/lib/Frontend/CompilerInvocation.cpp Outdated Show resolved Hide resolved

mdtoguchi reviewed Feb 24, 2021

View reviewed changes

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

premanandrao reviewed Feb 24, 2021

View reviewed changes

clang/test/CodeGenSYCL/loop_unroll.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/intel-fpga-mem-builtin.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/intel-fpga-reg.cpp Outdated Show resolved Hide resolved

Corrections based on review feedback + fixes.

e09bb27

Adds back -fno-sycl as a noop. Removes the diagnostic about passing -sycl-std= but not -fsycl-is-device or -fsycl-is-host. Fixes several tests, but not all of them yet.

premanandrao previously approved these changes Feb 24, 2021

View reviewed changes

Fix the frontend unit test behavior.

2208322

If -sycl-std is passed to the driver without -fsycl, then it is not passed to the -cc1 invocation.

AaronBallman dismissed premanandrao’s stale review via 2208322 February 24, 2021 21:49

mdtoguchi previously approved these changes Feb 24, 2021

View reviewed changes

premanandrao previously approved these changes Feb 24, 2021

View reviewed changes

More modifications to see if I can get tests to pass.

0ea5a5b

AaronBallman dismissed stale reviews from premanandrao and mdtoguchi via 0ea5a5b February 25, 2021 15:21

AaronBallman added 4 commits February 25, 2021 07:30

Merge remote-tracking branch 'upstream/sycl' into cc1-options

0e275eb

Fix formatting; NFC

d1caf86

Repair this test.

04fc3c4

Speculatively changing this test.

87c9a75

It doesn't make sense to pass -fsycl -Xclang -fsycl-is-host because that makes an invalid device compilation pass that has -fsycl-is-host and -fsycl-is-device at the same time.

AaronBallman dismissed elizabethandrews’s stale review via 2663a32 March 4, 2021 18:36

elizabethandrews previously approved these changes Mar 4, 2021

View reviewed changes

mdtoguchi previously approved these changes Mar 4, 2021

View reviewed changes

Removing the SYCL version as test behavior should be invariant

5da2fed

AaronBallman dismissed stale reviews from mdtoguchi and elizabethandrews via 5da2fed March 5, 2021 13:09

elizabethandrews previously approved these changes Mar 5, 2021

View reviewed changes

mdtoguchi previously approved these changes Mar 5, 2021

View reviewed changes

premanandrao previously approved these changes Mar 8, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/sycl' into cc1-options

0910ca9

AaronBallman dismissed stale reviews from premanandrao, mdtoguchi, and elizabethandrews via 0910ca9 March 9, 2021 13:32

elizabethandrews previously approved these changes Mar 9, 2021

View reviewed changes

bader reviewed Mar 9, 2021

View reviewed changes

Test this file only in host mode, not in both modes.

ef21e24

AaronBallman dismissed elizabethandrews’s stale review via ef21e24 March 9, 2021 13:53

Add an include path to the test to fix failures.

0b98654

mdtoguchi previously approved these changes Mar 9, 2021

View reviewed changes

elizabethandrews previously approved these changes Mar 9, 2021

View reviewed changes

Revert changes back to testing the file in both modes.

a4163d8

AaronBallman dismissed stale reviews from elizabethandrews and mdtoguchi via a4163d8 March 9, 2021 19:18

bader merged commit a539197 into intel:sycl Mar 10, 2021

AaronBallman mentioned this pull request Mar 10, 2021

id_queries_fit_int.cpp testing confusion #3335

Closed

smanna12 mentioned this pull request Mar 10, 2021

[SYCL][FPGA] Refactor of [[intel::max_global_work_dim()]] attribute #3326

Merged

premanandrao mentioned this pull request Mar 18, 2021

[SYCL] Cast to correct address space when a function object is passed #3303

Merged

mdtoguchi mentioned this pull request May 20, 2021

[SYCL][Driver] Need to duplicate options to be passed to both host and device compilers. #1808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Rework the way we handle SYCL -cc1 arguments. #3259

[SYCL] Rework the way we handle SYCL -cc1 arguments. #3259

AaronBallman commented Feb 24, 2021

AaronBallman commented Feb 24, 2021

erichkeane left a comment

premanandrao commented Feb 24, 2021

AaronBallman commented Feb 25, 2021

AaronBallman commented Feb 26, 2021

bader commented Mar 9, 2021

bader commented Mar 9, 2021

bader Mar 9, 2021

AaronBallman Mar 9, 2021

bader Mar 9, 2021

s-kanaev Mar 9, 2021

s-kanaev Mar 9, 2021 •

edited

Loading

bader Mar 9, 2021

AaronBallman Mar 9, 2021

s-kanaev Mar 10, 2021

AaronBallman Mar 10, 2021

s-kanaev Mar 10, 2021

		@@ -1,4 +1,4 @@
		// RUN: %clangxx -fsycl -Xclang -fsycl-is-host -fsycl-id-queries-fit-in-int -O0 -c -S -emit-llvm -o %t.ll %s
		// RUN: %clangxx -fsycl -fsycl-id-queries-fit-in-int -O0 -c -S -emit-llvm -o %t.ll %s

[SYCL] Rework the way we handle SYCL -cc1 arguments. #3259

[SYCL] Rework the way we handle SYCL -cc1 arguments. #3259

Conversation

AaronBallman commented Feb 24, 2021

AaronBallman commented Feb 24, 2021

erichkeane left a comment

Choose a reason for hiding this comment

premanandrao commented Feb 24, 2021

AaronBallman commented Feb 25, 2021

AaronBallman commented Feb 26, 2021

bader commented Mar 9, 2021

bader commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanaev Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-kanaev Mar 9, 2021 •

edited

Loading