[SYCL] Store the kernel object size in the integration header #5862

premanandrao · 2022-03-22T22:05:11Z

Sometimes when the host compiler is different from the device compiler,
it is possible that the size of the kernel object on the host will differ from the
size of the kernel object on the device. One such case is when constexpr
variables are used; compilers differ on whether they need to be captured and
this causes a discrepancy in the size of the kernel object and leads to failures
at runtime.

This change adds an interface to the integration header which returns the size
of the kernel object as computed on the device. The expectation is that the
library will assert the size of a kernel when compiling the host to detect
potential mismatches.

clang/test/CodeGenSYCL/int_header_kernelobjsize.cpp

clang/lib/Sema/SemaSYCL.cpp

clang/test/CodeGenSYCL/int_header_kernelobjsize.cpp

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2022-03-23T14:41:39Z

clang/include/clang/Sema/Sema.h

@@ -335,7 +335,8 @@ class SYCLIntegrationHeader {
  ///  Signals that subsequent parameter descriptor additions will go to
  ///  the kernel with given name. Starts new kernel invocation descriptor.
  void startKernel(const FunctionDecl *SyclKernel, QualType KernelNameType,
-                   SourceLocation Loc, bool IsESIMD, bool IsUnnamedKernel);
+                   SourceLocation Loc, bool IsESIMD, bool IsUnnamedKernel,
+                   unsigned long ObjSize);


unsigned long is likely the wrong type for these. This is 32 bits on windows. Note that the CharUnits underlying type is int64_t.

Thanks, done.

It doesn't appear to be here?

Sorry, git error on my part.

erichkeane · 2022-03-23T14:59:46Z

clang/lib/Sema/SemaSYCL.cpp

@@ -4777,6 +4782,10 @@ void SYCLIntegrationHeader::emit(raw_ostream &O) {
    O << "    return 0;\n";
    O << "#endif\n";
    O << "  }\n";
+    O << "  // Returns the size of the kernel object in bytes.\n";
+    O << "  __SYCL_DLL_LOCAL\n";
+    O << "  static constexpr int64_t getKernelSizeof() { return "


I was ok with getKernelSize (commented this was an old suggestion), but let @Fznamznon choose which she prefers.

Also, is int64_t going to be ok here? I dont know what amount of 'includes' we have in the header (since int64_t is a typedef).

I was ok with getKernelSize (commented this was an old suggestion), but let @Fznamznon choose which she prefers.

Okay, will wait for @Fznamznon.

Also, is int64_t going to be ok here? I dont know what amount of 'includes' we have in the header (since int64_t is a typedef).

Good point. I will check on both Linux and Windows.

Okay, will wait for @Fznamznon.

It LGTM now.

LIT tests show that Windows is not ok with that, see buildbot/sycl-win-x64-pr job result . We have a test that compiles integration header.
check-sycl tests are also not ok.

Looks like you'll have to make sure that int64_t is in the 'typedefs' section. You can look up the type in ASTContext to see if it is the same as LongLongTy or LongTy.

Regarding the name: I changed it to getKernelSize. getKernelSizeof felt a little weird.

Regarding the name: I changed it to getKernelSize. getKernelSizeof felt a little weird.

SGTM, I don't disagree.

erichkeane · 2022-03-23T20:04:00Z

clang/lib/Sema/SemaSYCL.cpp

@@ -4782,9 +4782,12 @@ void SYCLIntegrationHeader::emit(raw_ostream &O) {
    O << "    return 0;\n";
    O << "#endif\n";
    O << "  }\n";
+    StringRef ReturnType =


It might be useful to just put the typedef at the top of the int-header with the nullptr_t typedef (and a couple of others?) so that you can just use it? WDYT?

Oh! I see what you mean now. Sure, I can do that.

I actually don't see where we do that, but I could have sworn Jeff was working on that at one point? IF we ended up doing some of those typedefs that is perhaps my suggestion, otherwise this LGTM.

I thought you meant add them anew, but my recollection of Jeff's attempt is that he had to abandon it.
I will leave it as is for now.

Fznamznon

LGTM, thanks!

premanandrao · 2022-03-24T16:30:00Z

Thanks for the reviews.
@bader, the failure in OCL Gen9 suite (an unexpected pass) appears to be unrelated to my change. Other PRs (#5879 e.g.) show the same failure.

premanandrao · 2022-03-30T18:11:36Z

@bader or @intel/llvm-gatekeepers, this has been ready for a merge for a while now.

According to @Fznamznon's comments in #5899, the failure is being addressed in: https://github.com/intel/llvm-test-suite/pull/958/files

* sycl: (3343 commits) [SYCL][L0] Disable round-robin submissions to multiple CCSs (intel#5945) [SYCL][CUDA] Don't link pi_cuda against libsycl (intel#5908) [CI] Disable -Werror by default (intel#5889) [BuildBot] Uplift CPU/FPGAEMU RT version to 2022.13.3.0.16 (intel#5883) [SYCL][CUDA][libclc] Add support for atomic fp exchange and compare exchange (intel#5937) [SYCL] Fix device code outlining for static local variables (intel#5915) [SYCL][NFC] Refactor plugin CMakeLists.txt (intel#5799) [SPIR-V][Doc] Add JointMatrixWorkItemLengthINTEL instruction to joint matrix extension (intel#5781) [SYCL] Expand device_global map and make initialization order agnostic (intel#5902) [SYCL][CUDA] Add IPSCCP pass to O0 by default (intel#5900) [ESIMD] Disable ABI changes warnings in host compiler. (intel#5931) [SYCL] Make properties constructor constexpr (intel#5928) [NFC][SYCL] Fix static analysis warning (intel#5933) [CODEOWNERS][NFC] Assign code owners for CI scripts (intel#5873) [SYCL] Store the kernel object size in the integration header (intel#5862) [SYCL][ESIMD] Change esimd-verifier logic for detecting valid SYCL calls (intel#5914) [SYCL][CUDA][DOC] GettingStartedGuide.md to recommend cuda 11.6 (intel#5917) [SYCL][L0] Move command list cache usage under mutex (intel#5874) [SYCL][FPGA] Prepare future implementation of experimental pipe properties (intel#5886) [CI] Roll back intel driver to the latest version (intel#5925) ...

Store the kernel object size in the integration header

ca0e682

premanandrao requested a review from a team as a code owner March 22, 2022 22:05

premanandrao added 2 commits March 22, 2022 15:08

Fix clang-format errors

b9f758c

Fix clang-format errors (2)

83631c2

premanandrao requested review from erichkeane and romanovvlad March 22, 2022 22:14

Fznamznon reviewed Mar 23, 2022

View reviewed changes

clang/test/CodeGenSYCL/int_header_kernelobjsize.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/int_header_kernelobjsize.cpp Outdated Show resolved Hide resolved

Address code-review comments

8c27573

erichkeane reviewed Mar 23, 2022

View reviewed changes

Address more code review comments

d728d26

erichkeane reviewed Mar 23, 2022

View reviewed changes

premanandrao added 2 commits March 23, 2022 08:02

Missed one file

c95df69

Avoid int64_t and fix function name

fd7b9d3

erichkeane reviewed Mar 23, 2022

View reviewed changes

clang-format again

e1a845a

Fznamznon approved these changes Mar 24, 2022

View reviewed changes

erichkeane approved these changes Mar 24, 2022

View reviewed changes

smanna12 approved these changes Mar 24, 2022

View reviewed changes

romanovvlad approved these changes Mar 25, 2022

View reviewed changes

bader merged commit ef90e6a into intel:sycl Mar 31, 2022

[SYCL] Store the kernel object size in the integration header #5862

[SYCL] Store the kernel object size in the integration header #5862

Uh oh!

Conversation

premanandrao commented Mar 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Mar 24, 2022

Uh oh!

premanandrao commented Mar 30, 2022

Uh oh!

Uh oh!

Fznamznon Mar 23, 2022 •

edited

Loading

Fznamznon Mar 23, 2022 •

edited

Loading