Skip to content

[Driver][SYCL] Make /MD the default for -fsycl #2478

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 16, 2020
Merged

[Driver][SYCL] Make /MD the default for -fsycl #2478

merged 4 commits into from
Sep 16, 2020

Conversation

mdtoguchi
Copy link
Contributor

When using -fsycl on Windows, make /MD the default behavior. Any usage
of /MT will not be allowed and the driver will error upon usage.

elizabethandrews
elizabethandrews previously approved these changes Sep 15, 2020
View reviewed changes
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

v-klochkov
v-klochkov previously approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you.
One test for clang driver failed, probably the checks need to be updated there.

@v-klochkov
Copy link
Contributor

Just one minor comment: shouldn't this line be deleted? https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/MSVC.cpp#L379

@mdtoguchi
Copy link
Contributor Author

Just one minor comment: shouldn't this line be deleted? https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/MSVC.cpp#L379

Yes, it can be deleted, but there is no harm in it being there. I'll update.

@mdtoguchi
Copy link
Contributor Author

Test failure is not due to my change. The recent fix for default device libs for SYCL was probably tested and checked in before my change to add this test. I will update the test to match current expectations.

@mdtoguchi mdtoguchi requested a review from a team as a code owner September 15, 2020 22:08
v-klochkov
v-klochkov previously approved these changes Sep 15, 2020
View reviewed changes
@v-klochkov v-klochkov mentioned this pull request Sep 16, 2020
Merged
@mdtoguchi mdtoguchi force-pushed the private/mdtoguchi/win-MD-sycl-default branch from 099c30d to 4d13f91 Compare September 16, 2020 17:21
@mdtoguchi
Copy link
Contributor Author

rebased to address conflicts.

AGindinson
AGindinson approved these changes Sep 16, 2020
View reviewed changes
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 patch LGTM. For the bigger picture, what nasty scenarios does forbidding MT rule out?

@v-klochkov
Copy link
Contributor

The patch LGTM. For the bigger picture, what nasty scenarios does forbidding MT rule out?

This is to avoid situations like here: https://docs.microsoft.com/en-us/cpp/c-runtime-library/potential-errors-passing-crt-objects-across-dll-boundaries?view=vs-2019

I also wrote some more details in the commit message here: #2480

@v-klochkov v-klochkov merged commit d31184e into intel:sycl Sep 16, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Sep 17, 2020
Merge remote-tracking branch 'upstream/sycl' into accessor_property_list
7a51468 
* upstream/sycl: (405 commits)
  [SYCL] Implement new env var SYCL_DEVICE_FILTER (intel#2239)
  [Driver][SYCL] Make /MD the default for -fsycl (intel#2478)
  [SYCL]: basic support of contexts with multiple devices in Level-Zero (intel#2440)
  [SYCL] Fix LIT regression after 9dd18ca (intel#2481)
  [SYCL][L0] Kernel Destroy in piKernelRelease (intel#2475)
  [SYCL] Emit an aliased function only if it is used (intel#2430)
  [Driver][SYCL] Add defaultlib directive for sycl lib (intel#2464)
  [Driver][SYCL] Improve situations where .exe is added for AOT tools (intel#2467)
  [SYCL][L0]: Check Queue refcnt prior to using members in event wait/release (intel#2471)
  [SYCL] Unroll several loops in __init method accessor class (intel#2449)
  [SYCL][Doc] Add link to use pinned memory spec (intel#2463)
  [SYCL] Link SYCL device libraries by default. (intel#2400)
  Revert "[SYCL] XFAIL test blcoking pulldown"
  Avoid usage of deprecated "VectorType::getNumElements" (intel#737)
  Fix nullptr dereference (intel#741)
  Do not translate arbitrary precision operations without corresponding extensions (intel#714)
  Add Constrained Floating-Point Intrinsics support
  [SYCL] Take into account auxiliary cmake options for Level Zero loader
  [InstCombine] improve fold of pointer differences
  [InstCombine] add ptr difference tests; NFC
  ...
@v-klochkov v-klochkov mentioned this pull request Sep 21, 2020
Merged
v-klochkov added a commit to v-klochkov/llvm that referenced this pull request Mar 26, 2021
@v-klochkov
[SYCL] Check that apps using sycl also use dynamic C++ RT even withou…
47e9ed2 
…t -fsycl

With -fsycl switch the dynamic runtime is used.
The patches (intel#2478, intel#2480, intel#2497 implemented that with -fsycl.

This patch adds check that dynamic runtime is used when -fsycl is
not used. That is done by a static_assert in sycl/stl.hpp.

Also, lots of LIT tests had to be changed to comply with the new
requirement (apps must use dynamic C++ RT with use SYCL).

Signed-off-by: Vyacheslav N Klochkov <[email protected]>
v-klochkov added a commit that referenced this pull request Mar 31, 2021
@v-klochkov
[SYCL] Warning if use sycl without dynamic C++ RT on Windows (#2501)
d4180f4 
With -fsycl switch the dynamic runtime is used.
The patches (#2478, #2480, #2497 implemented that with -fsycl.

Applications using sycl headers and linked with sycl[d].dll must be linked
with dynamic C++ runtime on Windows even if compiled without -fsycl.
This patch adds a compile time warning emitted when wrong C++ runtime is used.

Signed-off-by: Vyacheslav N Klochkov <[email protected]>
iclsrc pushed a commit that referenced this pull request Apr 11, 2024
@vmaksimo @sys-ce-bb
CounterBuffer decoration is supported from 1.4 without extension (#2478)
3caac02 
Do nothing for now, as it's not used in translator.

Addresses p.6 of #2460

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@2c3b505879dcaf1
kbenzie added a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025
@kbenzie
Merge pull request intel#2478 from yingcong-wu/yc/1218-libcxx-mess
46bbad2 
[DeviceSanitizer] When link with libc++, link the gcc_s first.
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@kbenzie
Merge pull request #2478 from yingcong-wu/yc/1218-libcxx-mess
bc6aa73 
[DeviceSanitizer] When link with libc++, link the gcc_s first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@v-klochkov v-klochkov v-klochkov approved these changes

@AGindinson AGindinson AGindinson approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

@premanandrao premanandrao Awaiting requested review from premanandrao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mdtoguchi @v-klochkov @AGindinson @elizabethandrews