[SYCL] Allow SYCL_EXTERNAL for class member functions #1295

Fznamznon · 2020-03-12T15:43:10Z

This patch allows to apply sycl_device attribute and therefore SYCL_EXTERNAL macro to class member functions.

The same logic was applied to [[intel::device_indireclty_callable]].

erichkeane · 2020-03-12T15:45:51Z

clang/include/clang/Basic/AttrDocs.td

@@ -2098,7 +2098,7 @@ This attribute can only be applied to functions and indicates that the
 function must be treated as a device function and must be emitted even if it has
 no direct uses from other SYCL device functions. However, it cannot be applied
 to functions marked as 'static', functions declared within an anonymous
-namespace or class member functions.
+namespace or virtual/pure virtual class member functions.


I'm not sure the virtual/pure virtual is worth the differentiation. I'd say simply 'virtual' is sufficient. Same in the error message.

erichkeane · 2020-03-12T15:49:25Z

clang/lib/Sema/SemaDeclAttr.cpp

@@ -4429,9 +4429,9 @@ static void handleSYCLDeviceAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
        << AL << 0 /* static function or anonymous namespace */;
    return;
  }
-  if (isa<CXXMethodDecl>(FD)) {
+  if (FD->isVirtualAsWritten() || FD->isPure()) {


Do you have a case where the 'isPure' check is required? I don't know of any case where you can mark a function as pure but not as virtual. Additionally, I'd suggest just using this function: https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html#a1f488b7521f08ced46dce05c44cab5c5

I agree that using isVirtual function seems more elegant. I also can't recall cases when pure function is not virtual.
But then, I'm just curious, why isVirtual function also checks both isVirtualAsWritten and isPure internally. (see https://clang.llvm.org/doxygen/DeclCXX_8h_source.html#l01948 )

Ah, because Microsoft. __interface is a markup in MSVC (https://docs.microsoft.com/en-us/cpp/cpp/interface?view=vs-2019) that makes every function in it pure, but not marked virtual as written :)

For standards compliant C++, only the 'isvirtualaswritten' and size_overridden_methods checks actually are necessary.

erichkeane · 2020-03-12T15:50:21Z

clang/test/SemaSYCL/sycl-device.cpp

+  virtual int foo() {}
+
+  __attribute__((sycl_device)) // expected-error {{'sycl_device' attribute cannot be applied to a virtual or pure virtual class member function}}
+  virtual int bar() = 0;


Add a test for override/final as well! Additionally, cases where the class itself is 'final'.

I also wonder if we should ALLOW final versions of the function (or when the class is marked final).

I suppose, technically we could allow any stuff which do not bring virtual table usage into resulting LLVM IR module.
I had some interesting observation:
First, any call of virtual function doesn't bring vtable usage to IR if it was done without using of pointer to the base class, i.e.

struct Bar { virtual void bar1() {} }; __attribute__((sycl_device)) void FOO() { Bar b; b.bar1(); }

But, such calls will be diagnosed by our diagnostic in SemaSYCL here

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 226 in 092367b

SemaRef.Diag(e->getExprLoc(), diag::err_sycl_restrict)

.

Second, the diagnostic in SemaSYCL always triggered by the functions marked as override and final if they were marked as virtual in base class.
BTW if you are using function marked as final but through pointer to the base class, vtable usage will appear in resulting device code module anyway.
Another funny thing that the diagnostic implemented in place where attribute handling happens i.e. inside handleSYCLDeviceAttr function triggers only if function is explicitly marked as virtual, so, if you, for example, just override some virtual function and do not leave virtual keyword near it's declaration/definition, it will be possible to add sycl_kernel attribute to it, but calling of this function will be prohibited by the SemaSYCL diagnostic. I guess this happens because SemaSYCL diagnostic happens much more later when more information is presented.

The point to disallow sycl_kernel attribute for virtual function was to improve user experience, I guess, because virtual functions cannot be called on device, which makes marking them with sycl_device useless.
But I suppose if this diagnostic is not so efficient and it's behavior can't be aligned with the diagnostic which validates actual virtual function calls, we can do not implement it to do not confuse users. Because now it looks like this:

struct Bar { virtual void bar1() {} }; struct Bar1 : Bar { __attribute__((sycl_device)) void bar1() final {} // no diagnostic }; __attribute__((sycl_device)) void FOO() { Bar1 b; b.bar1(); // sycl-device.cpp:65:5: error: SYCL kernel cannot call a virtual function // b.bar1(); }

As a conclusion, I'm asking. @AlexeySachkov , @erichkeane is it ok if we won't implement diagnostic which disallow usage of sycl_kernel/indirectly_callable attirubute for virtual functions.

Please NOTE: my observation about that diagnostic for virtual function calls triggers even for cases which do not bring vtable usage to device code is not in scope of this PR.

Hmm... yeah, I had forgotten about the parts where we already know the type.

Bar &b =...; b.bar1();

Should definitely diagnose. However, any non-ref/pointer types already know their type. Anything marked 'override' should still obviously diagnose, BUT, this should not:

Bar1 &b=...; b.bar1();

Because bar1 is final in Bar1, I would expect us to KNOW the call and not need to use the vtable ( though I am not sure we CAN do this, I'm not sure if this sitll uses the vtable).

Another example:

struct Bar2 : Bar { __attribute___((sycl_device)) void bar1() override {} /// should still diagnose.

As I said, problems with diagnosing of virtual function calls is not in scope of this PR. I just captured my observations while adding new test cases and exploring behavior.
This PR is about SYCL_EXTERNAL and therefore about __attribute___((sycl_device))
Such case can't be diagnosed in a way I use now:

struct Bar2 : Bar { __attribute___((sycl_device)) void bar1() override {}

Because inside handleSYCLDeviceAttr function we don't have enough information yet, so here diagnostic happens only if function explicitly marked with virtual keyword. But once this function is called, the call will be diagnosed by our old diagnostic from SemaSYCL because it happens later with more information.
So, I propose do not diagnose sycl_device usage on virtual functions, because such diagnostic is not sufficient.

Right, I'm exploring the problem space here... I don't think skipping this diagnostic is a bad idea if we can properly count on diagnosing calls.

erichkeane

Generally OK, I would just like a few additional codegen tests, otherwise LGTM.

erichkeane · 2020-03-16T13:15:57Z

clang/test/SemaSYCL/sycl-device.cpp

+};
+
+class B {
+public:


Can you add tests for what the behavior (I think from a CodeGen perspective) of what happens to:

override functions marked with these attributes (where the 'base' does NOT have them)
override functions marked with these attributes (where hte base DOES have them)
unmarked override functions (where the base DOES have them)
marked/unmarked 'final' functions in the above.

Sure, added for both attributes.

This patch allows to apply `SYCL_EXTERNAL` to class member functions, but only if they weren't declared as virtual or as pure virtual. The same logic was applied to `[[intel::device_indireclty_callable]]`. Signed-off-by: Alexey Sachkov <[email protected]>

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Moved device code outlining to CodeGen because the old scheme in combination with SYCL_EXTERNAL implementation for methods triggered assertions Signed-off-by: Mariya Podchishchaeva <[email protected]>

erichkeane · 2020-03-17T17:50:02Z

@romanovvlad idk what that means. I reviewed the patch and am waiting on the committer to update the patch.

romanovvlad · 2020-03-17T20:36:09Z

@romanovvlad idk what that means. I reviewed the patch and am waiting on the committer to update the patch.

We use assignee field to indicate one person who is responsible
for reviewing a patch and is supposed to make an ultimate approve.

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Fznamznon · 2020-03-18T08:10:05Z

BTW, applying sycl_device (and SYCL_EXTERNAL) attribute to constructors/destructors triggered assertions. This happened because constructors are emitted in not the same way as regular functions and our old scheme of device code outlining forced them to be codegened twice. To fix this I moved device code outlining process to the CodeGen, this appoach helped me to fix assertions and I think it's better aligned with what community does for OpenMP device code outlining.
We don't need to implicitly apply sycl_device attribute to all declarations anymore. Calling of addSyclDeviceDecl also is required only for generated OpenCL kernels.

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/CodeGenSYCL/sycl-device.cpp

erichkeane

LGTM, looks like @bader might have some opens, so once he's ok so am I.

…intel#1295)

Fznamznon requested review from AlexeySachkov, erichkeane and sndmitriev March 12, 2020 15:43

bader changed the title ~~[SYCL] Update usage rules of SYCL_EXTERNAL~~ [SYCL] Allow SYCL_EXTERNAL for class member functions Mar 12, 2020

erichkeane reviewed Mar 12, 2020

View reviewed changes

Fznamznon requested a review from erichkeane March 16, 2020 08:00

erichkeane reviewed Mar 16, 2020

View reviewed changes

AlexeySachkov and others added 6 commits March 17, 2020 13:29

Extend the test

9fdd4a1

Signed-off-by: Mariya Podchishchaeva <[email protected]>

[SYCL] Erase diagnosing for virtual functions

f762faa

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Apply clang format to the test

cd1b06c

Signed-off-by: Mariya Podchishchaeva <[email protected]>

More clang format

2aef157

Signed-off-by: Mariya Podchishchaeva <[email protected]>

[SYCL] Move device code outlining to CodeGen, add tests

2225ffd

Moved device code outlining to CodeGen because the old scheme in combination with SYCL_EXTERNAL implementation for methods triggered assertions Signed-off-by: Mariya Podchishchaeva <[email protected]>

romanovvlad assigned erichkeane Mar 17, 2020

Fznamznon added 3 commits March 18, 2020 08:08

Do not add sycl_device to global vars too

28ca593

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Apply clang-format to the tests

a050731

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Apply comments

1c3036c

Signed-off-by: Mariya Podchishchaeva <[email protected]>

Fznamznon force-pushed the private/mpodchis/syclext-1 branch from 407a845 to 2225ffd Compare March 18, 2020 07:48

Fznamznon requested a review from bader March 18, 2020 08:12

bader reviewed Mar 18, 2020

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

bader reviewed Mar 18, 2020

View reviewed changes

clang/test/CodeGenSYCL/sycl-device.cpp Outdated Show resolved Hide resolved

clang/test/CodeGenSYCL/sycl-device.cpp Show resolved Hide resolved

clang/test/CodeGenSYCL/sycl-device.cpp Show resolved Hide resolved

erichkeane reviewed Mar 18, 2020

View reviewed changes

AlexeySachkov mentioned this pull request Mar 19, 2020

[WIP][SYCL] Update usage rules of SYCL_EXTERNAL #962

Closed

bader approved these changes Mar 20, 2020

View reviewed changes

bader mentioned this pull request Mar 20, 2020

[SYCL] Code not using virtual functions relies on function pointers #1361

Closed

bader merged commit 3baec18 into intel:sycl Mar 22, 2020

aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023

[SYCL] Update tests with changes in device selector exception message (…

a520d80

…intel#1295)

[SYCL] Allow SYCL_EXTERNAL for class member functions #1295

[SYCL] Allow SYCL_EXTERNAL for class member functions #1295

Uh oh!

Conversation

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

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!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

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!

erichkeane commented Mar 17, 2020

Uh oh!

romanovvlad commented Mar 17, 2020

Uh oh!

Fznamznon commented Mar 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fznamznon commented Mar 12, 2020 •

edited

Loading