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.

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

[SYCL][UX] Diagnostic for undefined device functions #1026

Merged
merged 58 commits into from
Mar 10, 2020
Merged

[SYCL][UX] Diagnostic for undefined device functions #1026

merged 58 commits into from
Mar 10, 2020

Conversation

s-kanaev
Copy link
Contributor

@s-kanaev s-kanaev commented Jan 20, 2020 •
edited
Loading

Brand new diagnostic introduced: "call an undefined function w/o SYCL_EXTERNAL attribute"
The diagnostic is displayed for functions definitions of which are not available when its declaration is not marked with SYCL_EXTERNAL attribute if and only if the method is found in callgraph (i.e. it will be called during runtime).

For this purpose there is a hack: attribute __SYCL__HAS_DEFINITION__ is used to suppress the diagnostic for __spirv_* and __spirv_ocl_* functions.

Standard library functions implementation is either of:

  1. header-only solution: implementation is provided as inline/static function in one of SYCL headers
  2. implementation is provided as a SPIRV.

The patch does not provide a facility for the second approach. Anyway, the approach should be smth similar to already used in devicelib.

@s-kanaev s-kanaev changed the title [SYCL] [UX] Kernel's call whitelisting [SYCL] [UX] Diagnostic for undefined kernel functions Jan 20, 2020
@Fznamznon
Copy link
Contributor

Diagnostic for undefined kernel functions -> Diagnostic for undefined device functions

AlexeySachkov
AlexeySachkov reviewed Jan 20, 2020
View reviewed changes
sycl/include/CL/__spirv/spirv_ops.hpp Outdated Show resolved Hide resolved
sycl/include/CL/__spirv/spirv_ops.hpp Outdated Show resolved Hide resolved
sycl/include/CL/sycl/detail/builtins.hpp Outdated Show resolved Hide resolved
sycl/include/CL/sycl/detail/builtins.hpp Outdated Show resolved Hide resolved
@bader bader changed the title [SYCL] [UX] Diagnostic for undefined kernel functions [SYCL][UX] Diagnostic for undefined kernel functions Jan 20, 2020
erichkeane
erichkeane reviewed Jan 20, 2020
View reviewed changes
clang/include/clang/Basic/DiagnosticSemaKinds.td Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved
@d86thoffner
Copy link

As long as we can circumvent the check with SYCL_EXTERNAL for functions we want to deal with in the FPGA device compiler, I think we are good from the FPGA side.

@s-kanaev s-kanaev changed the title [SYCL][UX] Diagnostic for undefined kernel functions [SYCL][UX] Diagnostic for undefined device functions Jan 20, 2020
@s-kanaev
Copy link
Contributor Author

As long as we can circumvent the check with SYCL_EXTERNAL for functions we want to deal with in the FPGA device compiler, I think we are good from the FPGA side.

@d86thoffner, as far as I understand both approaches from the description of this PR are OK for you, aren't they?

@d86thoffner
Copy link

Yes, as I tried to say, looks to me like this is not triggered for functions marked SYCL_EXTERNAL.
Which means that we can get function calls into the FPGA backend compiler even if the frontend isn't aware of whether they have a definition or not.
If so we are OK

erichkeane
erichkeane reviewed Jan 21, 2020
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

Clang changes look fine delta @Fznamznon format comments.

bader
bader reviewed Feb 28, 2020
View reviewed changes
clang/lib/Sema/SemaDecl.cpp Outdated Show resolved Hide resolved
Sergey Kanaev added 2 commits February 28, 2020 14:53
[SYCL] Fix some code-style issues
a806343 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
[SYCL] Fix some code-style issues
cdfee11 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>
@s-kanaev s-kanaev force-pushed the private/s-kanaev/function-whitelist branch from 9df6f9d to cdfee11 Compare February 28, 2020 11:54
@s-kanaev s-kanaev requested a review from bader February 28, 2020 14:01
Fznamznon
Fznamznon previously approved these changes Feb 28, 2020
View reviewed changes
rolandschulz
rolandschulz reviewed Feb 28, 2020
View reviewed changes
clang/lib/Sema/SemaDecl.cpp
@@ -17940,6 +17940,12 @@ Decl *Sema::getObjCDeclContext() const {
}

Sema::FunctionEmissionStatus Sema::getEmissionStatus(FunctionDecl *FD) {
// Due to SYCL functions are template we check if they have appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to be a full sentence. Word missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't see what is missing in "Due to SYCL functions are template we check if they have appropriate attribute prior to checking if it is a template"

Fznamznon
Fznamznon previously approved these changes Mar 10, 2020
View reviewed changes
bader
bader reviewed Mar 10, 2020
View reviewed changes
clang/test/SemaSYCL/sycl-restrict.cpp Outdated Show resolved Hide resolved
clang/test/SemaSYCL/sycl-varargs-cconv.cpp Outdated Show resolved Hide resolved
clang/test/SemaSYCL/call-to-undefined-function.cpp Outdated Show resolved Hide resolved
Sergey Kanaev added 2 commits March 10, 2020 14:01
@s-kanaev s-kanaev requested review from Fznamznon and bader March 10, 2020 12:08
@s-kanaev @bader
[SYCL] Fix typo
98f3ef3 
Signed-off-by: Sergey Kanaev <sergey.kanaev@intel.com>

Co-Authored-By: Alexey Bader <alexey.bader@intel.com>
@bader bader merged commit a3b340b into intel:sycl Mar 10, 2020
@s-kanaev s-kanaev deleted the private/s-kanaev/function-whitelist branch March 12, 2020 13:08
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 12, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/imag…
c04fe4f 
…e_api_test

* origin/sycl:
  [SYCL][NFC] Fix static code analysis concerns (intel#1283)
  [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277)
  [SYCL][CUDA] Implement the program kernel names query (intel#1248)
  [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261)
  [SYCL][UX] Diagnostic for undefined device functions (intel#1026)
  [SYCL] Reverse reqd_work_group_size attribute (intel#1234)
  [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249)
  [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129)
alexbatashev added a commit to alexbatashev/llvm that referenced this pull request Mar 13, 2020
@alexbatashev
Merge remote-tracking branch 'otcshare/sycl' into private/abatashe/po…
f78a60b 
…st_commit

* otcshare/sycl: (469 commits)
  [SYCL] Implement thread-local storage restriction (intel#1281)
  [Driver][SYCL][FPGA] Adjust the output location for the project report (intel#1278)
  [SYCL][NFC] Fix static code analysis concerns (intel#1283)
  [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277)
  [SYCL][CUDA] Implement the program kernel names query (intel#1248)
  [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261)
  [SYCL][UX] Diagnostic for undefined device functions (intel#1026)
  [SYCL] Reverse reqd_work_group_size attribute (intel#1234)
  [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249)
  [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129)
  [SYCL] Add buffer dimensions restriction (intel#1147)
  [SYCL][NFC] Update copyright header in handler files (intel#1271)
  [SYCL][NFC] Format the code with clang-format
  [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228)
  [SYCL][NFC] Fix doxygen warnings (intel#1270)
  [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268)
  [SYCL][NFC] Fix a misleading comment regarding the SYCL flow (intel#1266)
  Change capability for SpecId decoration
  README.md: Mention retrieving llvm archive signatures
  travis: Restore macOS builds
  ...
@Pennycook Pennycook mentioned this pull request Mar 13, 2020
Merged
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 15, 2020
Merge remote-tracking branch 'origin/sycl' into private/abatashe/host…
7bbb1ec 
…_accessor_refactor

* origin/sycl: (454 commits)
  [SYCL][NFC] Fix static code analysis concerns (intel#1283)
  [SYCL] Fix the test/basic_tests/buffer/subbuffer.cpp (intel#1277)
  [SYCL][CUDA] Implement the program kernel names query (intel#1248)
  [SYCL] Honor the LLVM_LIBDIR_SUFFIX variable at installation time (intel#1261)
  [SYCL][UX] Diagnostic for undefined device functions (intel#1026)
  [SYCL] Reverse reqd_work_group_size attribute (intel#1234)
  [SYCL] Rename project to oneAPI DPC++ Compiler (intel#1249)
  [SYCL][XPTI] Instrumentation of SYCL runtime with XPTI (intel#1129)
  [SYCL] Add buffer dimensions restriction (intel#1147)
  [SYCL][NFC] Update copyright header in handler files (intel#1271)
  [SYCL][NFC] Format the code with clang-format
  [SYCL][Test] Fix SYCL library location path for LIT tests (intel#1228)
  [SYCL][NFC] Fix doxygen warnings (intel#1270)
  [SYCL][CUDA] Add the CUDA backend to the deploy-sycl-toolchain target (intel#1268)
  Change capability for SpecId decoration
  README.md: Mention retrieving llvm archive signatures
  travis: Restore macOS builds
  Fix DebugInfo creation after LLVM change 7a42bab
  Add more missing mixes
  Add missing fixes
  ...
@bader bader mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader approved these changes

@Fznamznon Fznamznon Fznamznon approved these changes

@andykaylor andykaylor Awaiting requested review from andykaylor

@mikerice1969 mikerice1969 Awaiting requested review from mikerice1969

@romanovvlad romanovvlad Awaiting requested review from romanovvlad

@asavonic asavonic Awaiting requested review from asavonic

@garimagu garimagu Awaiting requested review from garimagu

@AlexeySachkov AlexeySachkov Awaiting requested review from AlexeySachkov

@erichkeane erichkeane Awaiting requested review from erichkeane

@AGindinson AGindinson Awaiting requested review from AGindinson

@rolandschulz rolandschulz Awaiting requested review from rolandschulz

Assignees

@Fznamznon Fznamznon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@s-kanaev @Fznamznon @d86thoffner @bader @rolandschulz @asavonic @AlexeySachkov @erichkeane @AGindinson