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

Can we drop target triple environment component -sycldevice? #3534

Closed
bader opened this issue Apr 12, 2021 · 2 comments · Fixed by #3929
Closed

Can we drop target triple environment component -sycldevice? #3534

bader opened this issue Apr 12, 2021 · 2 comments · Fixed by #3929
Assignees
Labels
enhancement New feature or request upstream This change is related to upstreaming SYCL support to llorg.

Comments

@bader
Copy link
Contributor

bader commented Apr 12, 2021

One of the use cases for this target triple component is configuring SPIR target address space map.
During discussion in https://reviews.llvm.org/D89909 comments we decided to implement a different approach, which doesn't require adding environment component for SYCL.
I'd like to gather to all existing use cases for -sycldevice and check if it's really necessary.

Use cases:

  1. NVPTX
  • We have two passes added in llvm/lib/Target/NVPTX/NVPTXTargetMachine.cpp to process GlobalOffset and Local accessors. These are added to the pipeline only if module has sycldevice environment component. @Naghasan, would be safe to enable them by default?
  • NVPTX target configures TLSSupported for SYCL. We should be able to overload adjust member function and uses language option for configuration. Done by [SYCL][NVPTX][NFC] Refactor NVPTX target configuration #3535.
  1. llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp - deletes "dead" SPIR kernel parameters. This is safe to do only when host part of the application setting SPIR kernel arguments is adjusted. TODO: check if we can enable this optimization w/o target triple modifications (e.g. metadata?)
  2. libclc uses a hack using -sycldevice to work around for [SYCL][Driver] fsycl-device-only does not offload, target triple is used to define programing model #1814. TODO: check if we can remove the hack as [SYCL][Driver] fsycl-device-only does not offload, target triple is used to define programing model #1814 is fixed.
  3. Driver. There are a lot of places where -sycldevice component is checked and I hope we can replace all these checks with the language mode check i.e. if -fsycl-[device,host]-only mode is set. @mdtoguchi, can we do this?
  4. AllocateTarget in clang/lib/Basic/Targets.cpp uses environment triple and host OS component for OS specific configuration. TODO: check if this needed and if not, revert. Probably should should be handled w/o checking environment triple within SPIRTargetInfo methods.
  5. Back-ends. DPC++ uses SPIR-V for Intel devices, which doesn't preserve -sycldevice, but we need to check if it's the case for other targets as well (e.g. NVPTX, Xilinx, Intel FPGA, ???).
  6. Documentation and LIT tests. These can be easily updated.
@bader bader added enhancement New feature or request upstream This change is related to upstreaming SYCL support to llorg. labels Apr 12, 2021
@bader bader self-assigned this Apr 12, 2021
@mdtoguchi
Copy link
Contributor

mdtoguchi commented Apr 12, 2021

@bader, looking through the driver, we should be able to make the adjustments to move away from anything -sycldevice and just check for isOffloading(OFK_SYCL). Would we also be removing the default triple setting of -sycldevice?

@bader
Copy link
Contributor Author

bader commented Apr 12, 2021

Great! Thanks for the update.
I think #3535 should remove (1) use cases for NVPTX target.

@bader bader linked a pull request Jun 12, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request upstream This change is related to upstreaming SYCL support to llorg.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants