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

[Driver][SYCL] Add diagnostic for bad argument with -fsycl-device-obj #15381

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

mdtoguchi
Copy link
Contributor

When using -fsycl-device-obj, we would effectively ignore any bad arguments and default to 'llvmir'. Add a diagnostic to inform the user of the bad argument and what we are doing with our default behavior.

When using -fsycl-device-obj, we would effectively ignore any bad
arguments and default to 'llvmir'. Add a diagnostic to inform the user
of the bad argument and what we are doing with our default behavior.
@@ -69,6 +69,13 @@
// RUN: | FileCheck -check-prefix=CHK-SYCL-TARGET %s
// CHK-SYCL-TARGET-NOT: error: SYCL target is invalid

/// -fsycl-device-obj argument check
// RUN: %clang -### -fsycl-device-obj=test -fsycl %s 2>&1 \
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 behavior when --offload-new-driver or --no-offload-new-driver is passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is the same. The use of --offload-new-driver vs --no-offload-new-driver does not change the need for the diagnostic or the behavior involved with -fsycl-device-obj.

StringRef ArgValue(DeviceObj->getValue());
SmallVector<StringRef, 2> DeviceObjValues = {"spirv", "llvmir"};
auto FoundArg =
std::find(DeviceObjValues.begin(), DeviceObjValues.end(), ArgValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a vector is used here, llvm::find_if can be used to instead of std::find and an if to simplify this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've simplified by using llvm::find (I found find_if to be just as 'complex' as the original)

@mdtoguchi
Copy link
Contributor Author

Testing issue is a XPASS which is not from these changes (and witnessed in other PRs)

Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

LGTM pending fix of typo.

FE change just adds a diagnostic

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved
@mdtoguchi
Copy link
Contributor Author

@intel/llvm-gatekeepers, this PR looks ready for merge - please take a look. Thanks!

@sarnex sarnex merged commit 81a9060 into intel:sycl Sep 16, 2024
13 checks passed
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.

5 participants