Skip to content

[SYCL][ESIMD] Prepare LLVM IR for OS spirv translator #2112

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 2 commits into from
Jul 22, 2020
Merged

[SYCL][ESIMD] Prepare LLVM IR for OS spirv translator #2112

merged 2 commits into from
Jul 22, 2020

Conversation

NikitaRudenkoIntel
Copy link
Contributor

No description provided.

AGindinson
AGindinson previously approved these changes Jul 14, 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 driver part LGTM

bader
bader requested changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

@NikitaRudenkoIntel, please, fix build failuires.

bader
bader previously approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@bader bader left a comment •
edited
Loading

Choose a reason for hiding this comment

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

llvm/lib/SYCLLowerIR/CMakeLists.txt changes look okay to me.

elizabethandrews
elizabethandrews previously approved these changes Jul 16, 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.

FE changes LGTM. I don't know the reason why metadata change from 6 to 0 is required but the change itself is fine.

I did not check the CMake changes since I am not very familiar with CMake.

@NikitaRudenkoIntel
Copy link
Contributor Author

The Khronos translator does not have source language "6". In vector compute patches we use other approach to decide on how to translate VC-specific things

@kbobrovs kbobrovs added the esimd Explicit SIMD feature label Jul 20, 2020
kbobrovs
kbobrovs previously approved these changes Jul 20, 2020
View reviewed changes
@kbobrovs
Copy link
Contributor

@AGindinson - please re-approve

AGindinson
AGindinson reviewed Jul 20, 2020
View reviewed changes
MrSidims
MrSidims reviewed Jul 20, 2020
View reviewed changes
clang/lib/Driver/ToolChains/SYCL.cpp
@@ -42,6 +42,8 @@ const char *SYCL::Linker::constructLLVMSpirvCommand(Compilation &C,
} else {
CmdArgs.push_back("-spirv-max-version=1.1");
CmdArgs.push_back("-spirv-ext=+all");
if (C.getArgs().hasArg(options::OPT_fsycl_esimd))
Copy link
Contributor

@MrSidims MrSidims Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not in a scope of this patch, but I think we shall add another option to enable translation of unknown to the translator intrinsics for users, for example: #2122

AGindinson
AGindinson previously approved these changes Jul 20, 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.

LGTM, thanks!

kbobrovs
kbobrovs previously approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

@NikitaRudenkoIntel, in DPC++ llvm-spirv is called through another "driver" - llvm-foreach. I suggested a change which should fix the clang/test/Driver/sycl-offload.c failure.

AGindinson
AGindinson previously approved these changes Jul 22, 2020
View reviewed changes
@kbobrovs
Copy link
Contributor

@bader, CUDA buildbot failed with

CMake Error at /usr/local/share/cmake-3.15/Modules/FindCUDA.cmake:700 (message):
Specify CUDA_TOOLKIT_ROOT_DIR
Call Stack (most recent call first):
/localdisk2/sycl_ci/buildbot/worker/Lit_With_Cuda/llvm.src/sycl/plugins/cuda/CMakeLists.txt:7 (find_package)

This does not seem related to this patch. Since it is approved and all other checks passed - do you think it can be merged?

@kbobrovs kbobrovs requested a review from bader July 22, 2020 08:25
@bader bader merged commit bc7cd5c into intel:sycl Jul 22, 2020
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@callumfare
Merge pull request #2112 from martygrant/martin/context-cts-spec-gap
cf86da8 
Improvements to align CTS and Spec for Context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrSidims MrSidims MrSidims left review comments

@kbobrovs kbobrovs kbobrovs left review comments

@AGindinson AGindinson AGindinson left review comments

@elizabethandrews elizabethandrews elizabethandrews left review comments

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

@mdtoguchi mdtoguchi Awaiting requested review from mdtoguchi

@premanandrao premanandrao Awaiting requested review from premanandrao

@bader bader Awaiting requested review from bader

Assignees
No one assigned
Labels
esimd Explicit SIMD feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@NikitaRudenkoIntel @kbobrovs @bader @MrSidims @AGindinson @elizabethandrews