[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions. #1778

kbobrovs · 2020-05-29T06:30:12Z

SYCL_EXTERNAL functions are not included into the device declarations
set which is iterated during SYCL attribute propagation and checking.

This leads e.g. to the following code getting silently accepted:

[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
[[cl::intel_reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() {
  sg_size2();
}

SYCL_EXTERNAL function with reqd_sub_group_size 4 calls a function with reqd_sub_group_size 2. This shoud be an error. Existing implementation detects it only when there is a kernel in place of SYCL_EXTERNAL function

Signed-off-by: Konstantin S Bobrovsky konstantin.s.bobrovsky@intel.com

SYCL_EXTERNAL functions are not included into the device declarations set which is iterated during SYCL attribute propagation and checking. Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

AlexeySachkov · 2020-05-29T09:52:17Z

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp

+[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
+
+// expected-note@+2 {{conflicting attribute is here}}
+// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}


This diagnostic doesn't seem to be correct, because function it is pointing to is not a SYCL kernel

This diagnostic seems strange as well, why does it think that sg_size4 and sg_size2 are the same kernel?

Should this conflict be diagnosed on a call expression instead?

This diagnostic doesn't seem to be correct, because function it is pointing to is not a SYCL kernel

OK

This diagnostic seems strange as well, why does it think that sg_size4 and sg_size2 are the same kernel?

Why do you think "it think that sg_size4 and sg_size2 are the same kernel" - please elaborate.

Should this conflict be diagnosed on a call expression instead?

This is a question to existing design. I'm not changing anything in the diagnostics mechanism. I'm fixing existing bug where SYCL_EXTERNAL functions were missing in that diagnostics - I add them.

From the error message, it says "conflicting attributes applied to a SYCL kernel". From the notes, it specifies attributes on two separate declarations.

So by reading the error + notes, it implies that there are two conflicting attributes on a single kernel, yet they are on different functions.

This error message existed before - I did not invent it, I just used it for SYCL_EXTERNAL function. Do you suggest to fix prior problems?

We do this already? We have other conditions where we diagnose an error on a kernel about conflicting attributes, and then point to an attribute on another declaration?

Yep. SYCL attributes are propagated from called device function up to the kernel (AFAIK it is spec defined behavior which I find pretty magic), sometimes there are several functions called from kernel and their attribute values can conflict. Diagnosing happens here

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 1484 in 12d14e8

Diag(SYCLKernel->getLocation(),

Ah, well, the way we report that is awful. But not needing to be fixed in this patch. Thanks @Fznamznon. The error-message should likely be changed to be more clear about this if anyone ends up getting to it.

Fznamznon

I don't really see value in this diagnostic. Does application of intel_reqd_sub_group_size attribute to function which is not called from a kernel make sense?
Imagine that SYCL_EXTERNAL function has some intel_reqd_sub_group_size in the translation unit where it is defined and declaration of this function in the translation unit doesn't have intel_reqd_sub_group_size. Or it has some other value in intel_reqd_sub_group_size attribute. We cannot diagnose this in FE. FE can just miss propagation of this attribute to the kernel from
called SYCL_EXTERNAL function if user didn't mark declaration of it. I think there are a lot of other cases which cannot be handled/diagnosed by FE.

erichkeane · 2020-05-29T12:24:48Z

clang/include/clang/Sema/Sema.h

@@ -12556,7 +12556,7 @@ class Sema final {
 private:
  // We store SYCL Kernels here and handle separately -- which is a hack.
  // FIXME: It would be best to refactor this.
-  SmallVector<Decl*, 4> SyclDeviceDecls;
+  llvm::SetVector<Decl *> SyclDeviceDecls;


Why make this change? Switching to a setVector doesn't seem to help anything here, particularly when we are using it like a vector.

SetVector allows both deduplication and deterministic iteration order. The former is needed because SYCL_EXTERNAL functions can also be called from kernels, in which case addSyclDeviceDecl(FD) on vector would lead to duplicates. The latter is needed for deterministic compiler behavior.

Thanks for clarifying. Can you elaborate the problem better in the pull request message? I still don't have a good hold of the problem you're trying to solve.

It is all in the test case I added:

[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {} [[cl::intel_reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() { sg_size2(); }

SYCL_EXTERNAL function with reqd_sub_group_size 4 calls a function with reqd_sub_group_size 2. This shoud be an error. Existing implementation detects it only when there is a kernel in place of SYCL_EXTERNAL function, and silently generates incorrect code for SYCL_EXTERNAL.

SYCL_EXTERNAL == __attribute__((sycl_device)), BTW

erichkeane · 2020-05-29T12:27:22Z

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp

+[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
+
+// expected-note@+2 {{conflicting attribute is here}}
+// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}


This diagnostic seems strange as well, why does it think that sg_size4 and sg_size2 are the same kernel?

Should this conflict be diagnosed on a call expression instead?

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs · 2020-05-30T02:27:22Z

I don't really see value in this diagnostic. Does application of intel_reqd_sub_group_size attribute to function which is not called from a kernel make sense?
Imagine that SYCL_EXTERNAL function has some intel_reqd_sub_group_size in the translation unit where it is defined and declaration of this function in the translation unit doesn't have intel_reqd_sub_group_size. Or it has some other value in intel_reqd_sub_group_size attribute. We cannot diagnose this in FE. FE can just miss propagation of this attribute to the kernel from
called SYCL_EXTERNAL function if user didn't mark declaration of it. I think there are a lot of other cases which cannot be handled/diagnosed by FE.

I do see the value. This case can and should be diagnosed. Imagine that sg_size2 leads to UB if the subgroup size it is invoked with is not 2.

Fznamznon · 2020-06-01T11:23:14Z

I don't really see value in this diagnostic. Does application of intel_reqd_sub_group_size attribute to function which is not called from a kernel make sense?
Imagine that SYCL_EXTERNAL function has some intel_reqd_sub_group_size in the translation unit where it is defined and declaration of this function in the translation unit doesn't have intel_reqd_sub_group_size. Or it has some other value in intel_reqd_sub_group_size attribute. We cannot diagnose this in FE. FE can just miss propagation of this attribute to the kernel from
called SYCL_EXTERNAL function if user didn't mark declaration of it. I think there are a lot of other cases which cannot be handled/diagnosed by FE.

I do see the value. This case can and should be diagnosed. Imagine that sg_size2 leads to UB if the subgroup size it is invoked with is not 2.

I meant that I don't see value in propagation of attributes to SYCL_EXTERNAL function because it can lead to a lot of problems which we cannot handle/diagnose in FE.

kbobrovs · 2020-06-02T05:09:02Z

I meant that I don't see value in propagation of attributes to SYCL_EXTERNAL function because it can lead to a lot of problems which we cannot handle/diagnose in FE.

OK. The original comment contained objection to the diagnostics. If this really about propagation - I did not invent attributes propagation. BTW, I removed propagating sycl_explicit_simd, as agreed with @DenisBakhvalov

Fznamznon · 2020-06-02T07:48:58Z

I meant that I don't see value in propagation of attributes to SYCL_EXTERNAL function because it can lead to a lot of problems which we cannot handle/diagnose in FE.

OK. The original comment contained objection to the diagnostics. If this really about propagation - I did not invent attributes propagation. BTW, I removed propagating sycl_explicit_simd, as agreed with @DenisBakhvalov

You invented propagation up to SYCL_EXTERNAL function, as far as I understood.

kbobrovs · 2020-06-03T02:37:24Z

I meant that I don't see value in propagation of attributes to SYCL_EXTERNAL function because it can lead to a lot of problems which we cannot handle/diagnose in FE.

OK. The original comment contained objection to the diagnostics. If this really about propagation - I did not invent attributes propagation. BTW, I removed propagating sycl_explicit_simd, as agreed with @DenisBakhvalov

You invented propagation up to SYCL_EXTERNAL function, as far as I understood.

As I explained above, SYCL_EXTERNAL deserves similar error checking for attribute compatibility as kernels do and the test that comes with this patch shows the problematic case. If you have another opinion - please provide some justification please.

Fznamznon · 2020-06-03T11:50:51Z

I meant that I don't see value in propagation of attributes to SYCL_EXTERNAL function because it can lead to a lot of problems which we cannot handle/diagnose in FE.

OK. The original comment contained objection to the diagnostics. If this really about propagation - I did not invent attributes propagation. BTW, I removed propagating sycl_explicit_simd, as agreed with @DenisBakhvalov

You invented propagation up to SYCL_EXTERNAL function, as far as I understood.

As I explained above, SYCL_EXTERNAL deserves similar error checking for attribute compatibility as kernels do and the test that comes with this patch shows the problematic case. If you have another opinion - please provide some justification please.

Okay, I had some offline discussion with @AlexeySachkov regarding my concerns. It seems we do need some checks for attributes inconsistency, although we don't need attribute propagation. The current code for diagnosing nailed to propagation, so removing of propagation is not really in scope of your change.

Fznamznon

Code is OK, but I'd like @AlexeySachkov to approve first.

Fznamznon

Still is ok, @AlexeySachkov please take a look.

AlexeySachkov

Overall, LGTM. Two minor comments

AlexeySachkov · 2020-06-08T10:49:39Z

clang/test/SemaSYCL/reqd-sub-group-size-device.cpp

+[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
+
+// expected-note@+2 {{conflicting attribute is here}}
+// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}


Suggested change

// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}

// expected-error@+1 {{conflicting attributes applied to a SYCL kernel or SYCL_EXTERNAL function}}

AlexeySachkov · 2020-06-08T10:59:15Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1460,6 +1460,15 @@ void Sema::MarkDevice(void) {
  // it is recursive.
  MarkDeviceFunction Marker(*this);
  Marker.SYCLCG.addToCallGraph(getASTContext().getTranslationUnitDecl());
+
+  // Iterate through SYCL_EXTERNAL functions and add them to the device decls.
+  for (const auto &entry : *Marker.SYCLCG.getRoot()) {


IIRC, @Fznamznon pointed out that this won't add class member functions, which are marked as SYCL_EXTERNAL into list of device functions which needs to be checked.
This is not critical, I guess, but I think deserves double-checking and probably a TODO, that attribute handling should be improved for class member functions as well

I wasn't sure about it, so I double checked it locally, seems everything is fine with class members, but we could add a corresponding test in next patches.

Fixed an issue with comparing pointers to Attr objects rather than values passed to the attribute in source code. Refactored IntelReqdSubGroupSizeAttr class to store evaluated value of attribute value for further uses to avoid evaluating it each time we need that value to check something. The bug were uncovered after intel#1778 landed, but it had been introduced in intel#1807

Fixed an issue with comparing pointers to Attr objects rather than values passed to the attribute in source code. The bug were uncovered after #1778 landed, but it had been introduced in #1807

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@e06e46d

kbobrovs requested review from erichkeane and Fznamznon as code owners May 29, 2020 06:30

[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions.

d2e625b

SYCL_EXTERNAL functions are not included into the device declarations set which is iterated during SYCL attribute propagation and checking. Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

kbobrovs force-pushed the fe_extern_fix branch from 6a70a88 to d2e625b Compare May 29, 2020 07:17

AlexeySachkov reviewed May 29, 2020

View reviewed changes

Fznamznon reviewed May 29, 2020

View reviewed changes

erichkeane reviewed May 29, 2020

View reviewed changes

[SQUASH] Review comments

2cda43a

Signed-off-by: Konstantin S Bobrovsky <konstantin.s.bobrovsky@intel.com>

Fznamznon reviewed Jun 3, 2020

View reviewed changes

kbobrovs requested review from erichkeane, Fznamznon and AlexeySachkov June 6, 2020 02:30

Fznamznon approved these changes Jun 8, 2020

View reviewed changes

AlexeySachkov approved these changes Jun 8, 2020

View reviewed changes

erichkeane approved these changes Jun 8, 2020

View reviewed changes

bader merged commit 0098eab into intel:sycl Jun 8, 2020

AlexeySachkov mentioned this pull request Jun 16, 2020

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

Merged

kbobrovs deleted the fe_extern_fix branch July 30, 2020 12:30

bb-sycl pushed a commit that referenced this pull request Feb 1, 2023

Fix issue due to ignoring return value (#1778)

fbe17f1

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@e06e46d

	// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}
	// expected-error@+1 {{conflicting attributes applied to a SYCL kernel or SYCL_EXTERNAL function}}

[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions. #1778

[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions. #1778

Uh oh!

Conversation

kbobrovs commented May 29, 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!

Fznamznon 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!

Choose a reason for hiding this comment

Uh oh!

kbobrovs May 30, 2020 • 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!

kbobrovs May 30, 2020 • 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!

kbobrovs commented May 30, 2020

Uh oh!

Fznamznon commented Jun 1, 2020

Uh oh!

kbobrovs commented Jun 2, 2020

Uh oh!

Fznamznon commented Jun 2, 2020

Uh oh!

kbobrovs commented Jun 3, 2020

Uh oh!

Fznamznon commented Jun 3, 2020

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

Fznamznon left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbobrovs commented May 29, 2020 •

edited

Loading

kbobrovs May 30, 2020 •

edited

Loading

kbobrovs May 30, 2020 •

edited

Loading