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][E2E] Fix warnings from using GNU style options with MSVC compiler driver #15364

Merged
merged 4 commits into from
Sep 11, 2024

Conversation

ayylol
Copy link
Contributor

@ayylol ayylol commented Sep 11, 2024

Due to the addition of the -Werror flag, these warnings were causing test failures when compiling tests with clang-cl. This patch does the following:

  • Add -Wno-unused-command-line-argument to linking run lines. This is needed because when testing the MSVC driver the /EHsc flag is added to all run lines that call the compiler. This flag however is only needed at the compile stage so it is reported as unused if a run line is only linking.
  • Add new expansion for no optimizations %no_opt, which is either -O0 or /Od depending on the compiler driver.
  • Use the expansion %cxx_std_option in place of -std=
  • For flags that do not necessarily have an MSVC equivalent, prepend /clang: to the flag when using MSVC driver.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

esimd lgtm

@sarnex sarnex requested a review from a team September 11, 2024 16:47
@bader
Copy link
Contributor

bader commented Sep 11, 2024

  • Add -Wno-unused-command-line-argument to linking run lines. This is needed because when testing the MSVC driver the /EHsc flag is added to all run lines that call the compiler. This flag however is only needed at the compile stage so it is reported as unused if a run line is only linking.

Should we fix the driver instead of ignoring the warning? @intel/dpcpp-clang-driver-reviewers, FYI.

@ayylol
Copy link
Contributor Author

ayylol commented Sep 11, 2024

The failure on gen12 Linux is unrelated to the changes in this pr. Reported in #15341

@againull againull merged commit d3d9521 into intel:sycl Sep 11, 2024
11 of 12 checks passed
@ayylol ayylol deleted the cl-options branch September 11, 2024 20:49
@@ -6,5 +6,5 @@
// RUN: %{run} %t.out

// Test -O0 with `--offload-new-driver`
// RUN: %clangxx -O0 -fsycl -fsycl-targets=spir64-x86_64 %S/Inputs/aot.cpp
// RUN: %clangxx %no_opt -fsycl -fsycl-targets=spir64-x86_64 %S/Inputs/aot.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that %O0 would be a better name for this substitution.

The thing is that we have two different methods of disabling optimizations and no_opt is a bit confusing:

  • We have -O0 which disables both optimizations made by SYCL compiler and also records some metadata into device images which later leads to backend optimizations also being disabled
  • We also have -fno-sycl-early-optimizations, which only disables optimizations made by SYCL compiler, but preserves optimizations made by device compilers later

Even though optimization pipeline is very similar (if not the same) for both options, resulting LLVM IR will differ between them. -O0 will have optnone attached to functions, whilst -fno-sycl-early-optimizations won't result in such attribute

againull pushed a commit that referenced this pull request Sep 23, 2024
Changes the `%no_opt` expansion introduced in #15364, to `%O0` to reduce
confusion in between different methods of disabling optimizations.
againull pushed a commit that referenced this pull request Oct 11, 2024
Sets C++ version with lit expansion for run line that was missed in
#15364. To avoid warning due to using GCC style options with MSVC
driver.
againull pushed a commit that referenced this pull request Oct 21, 2024
Fixing the options for another test that slipped through the cracks of
#15364
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