[SYCL] Add template parameter support for intel_reqd_sub_group_size a… #1807

smanna12 · 2020-06-02T20:52:41Z

…ttribute

Signed-off-by: Soumi Manna soumi.manna@intel.com

clang/test/SemaOpenCL/invalid-kernel-attrs.cl

MrSidims

LGTM with few nits:

Please also add a test in CodeGenSYCL.

MrSidims · 2020-06-03T07:30:20Z

clang/lib/Sema/SemaDeclAttr.cpp

+      return;
+    }
+    int32_t ArgInt = ArgVal.getSExtValue();
+    if (ArgInt < 0) {


Suggested change

if (ArgInt < 0) {

if (ArgInt <= 0) {

MrSidims · 2020-06-03T07:30:47Z

clang/lib/Sema/SemaDeclAttr.cpp

+    int32_t ArgInt = ArgVal.getSExtValue();
+    if (ArgInt < 0) {
+      Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
+          << Attr.getAttrName() << /*non-negative*/ 1;


Suggested change

<< Attr.getAttrName() << /*non-negative*/ 1;

<< Attr.getAttrName() << /*positive*/ 0;

MrSidims · 2020-06-03T07:31:12Z

clang/lib/Sema/SemaDeclAttr.cpp

+          << Attr.getAttrName() << /*non-negative*/ 1;
+      return;
+    }
+    if (ArgInt == 0) {


Suggested change

if (ArgInt == 0) {

Fznamznon · 2020-06-03T08:35:14Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+static void instantiateIntelReqdSubGroupSizeAttr(
+    Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
+    const IntelReqdSubGroupSizeAttr *Attr, Decl *New) {
+  // The SubGroupSize expression is a constant expressions.
+  EnterExpressionEvaluationContext Unevaluated(
+      S, Sema::ExpressionEvaluationContext::ConstantEvaluated);
+  ExprResult Result = S.SubstExpr(Attr->getSubGroupSize(), TemplateArgs);
+  if (!Result.isInvalid())
+    S.addIntelReqdSubGroupSizeAttr(New, *Attr, Result.getAs<Expr>());
+}


This comment shouldn't block this particular patch since is a general problem, but I want to highlight it.
If you take a look at the code above, you will feel deja vu, because there are a ton of same copy-pasted functions for other SYCL attributes (most of them for FPGA). I think we have to get rid of this copy-paste and unify same code across different attribute handling.

Fznamznon · 2020-06-03T08:35:41Z

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

+static void instantiateIntelReqdSubGroupSizeAttr(
+    Sema &S, const MultiLevelTemplateArgumentList &TemplateArgs,
+    const IntelReqdSubGroupSizeAttr *Attr, Decl *New) {
+  // The SubGroupSize expression is a constant expressions.


Suggested change

// The SubGroupSize expression is a constant expressions.

// The SubGroupSize expression is a constant expression.

erichkeane · 2020-06-03T20:48:39Z

Please don't force push unless absolutely necessary, it removes the conversations. That said, it seems the latest force-push doesn't do all the comments above.

smanna12 · 2020-06-03T20:53:27Z

Please don't force push unless absolutely necessary, it removes the conversations. That said, it seems the latest force-push doesn't do all the comments above.

Sorry about this. I did not know that this removes the conversation.

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

erichkeane · 2020-06-04T03:17:50Z

You seem to have force-pushed again. Please don't.

smanna12 · 2020-06-04T03:23:14Z

You seem to have force-pushed again. Please don't.

Sorry for the force-pushed again. Does " git push origin " command work.? It is not allowing me to push the patch.

smanna12 · 2020-06-04T05:18:58Z

I am not sure why clang-format check is failing with invalid-kernel-attr.cl file.

mlychkov · 2020-06-04T08:05:45Z

I am not sure why clang-format check is failing with invalid-kernel-attr.cl file.

-kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
+kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15() {}                                                 // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
 kernel __attribute__((intel_reqd_sub_group_size(8))) __attribute__((intel_reqd_sub_group_size(16))) void kernel16() {}  //expected-warning{{attribute 'intel_reqd_sub_group_size' is already applied with different parameters}}

Looks like it wants you to align comment indentation for kernel15 with the comment on next string.
Maybe inserting of empty line between kernel15 and kernel16 helps to cheat clang-format :)

mlychkov · 2020-06-04T10:30:58Z

You seem to have force-pushed again. Please don't.

Sorry for the force-pushed again. Does " git push origin " command work.? It is not allowing me to push the patch.

Do you store new changes into the same commit via amend option? git push doesn't work in this case. You should add new changes into separate commit.

erichkeane · 2020-06-04T12:40:29Z

clang/test/SemaOpenCL/invalid-kernel-attrs.cl

@@ -35,7 +35,7 @@ kernel __attribute__((reqd_work_group_size(1,0,2))) void kernel12(){} // expecte
 kernel __attribute__((reqd_work_group_size(0,1,2))) void kernel13(){} // expected-error {{'reqd_work_group_size' attribute must be greater than 0}}

 __attribute__((intel_reqd_sub_group_size(8))) void kernel14(){} // expected-error {{attribute 'intel_reqd_sub_group_size' can only be applied to an OpenCL kernel}}
-kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute must be greater than 0}}
+kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}


This has changed because you're now treating this as a signed value, whereas the previous code treated it as unsigned, and thus a different error message makes sense.

Please add a test with a negative value as well to make sure that doesn't change again. I realize you have one in the template version, but I'd like one right next to this one to show intent.

I have added a test with a negative value. But it seems like changing again. I will try to add inserting of empty line to fix the clang-format check error.

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 · 2020-06-04T15:36:01Z

Sorry about the multiple submission. I am still trying to figure out why clang-format is failing even with inserting of empty line.

bader · 2020-06-04T15:38:55Z

Sorry about the multiple submission. I am still trying to figure out why clang-format is failing even with inserting of empty line.

https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting - this should help you to fix the formatting.

smanna12 · 2020-06-04T15:40:56Z

Sorry about the multiple submission. I am still trying to figure out why clang-format is failing even with inserting of empty line.

https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting - this should help you to fix the formatting.

@bader Thanks. I will try this.

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 · 2020-06-04T19:21:13Z

I am not sure why clang-format check is failing with invalid-kernel-attr.cl file.

-kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15(){} // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
+kernel __attribute__((intel_reqd_sub_group_size(0))) void kernel15() {}                                                 // expected-error {{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
 kernel __attribute__((intel_reqd_sub_group_size(8))) __attribute__((intel_reqd_sub_group_size(16))) void kernel16() {}  //expected-warning{{attribute 'intel_reqd_sub_group_size' is already applied with different parameters}}

Looks like it wants you to align comment indentation for kernel15 with the comment on next string.
Maybe inserting of empty line between kernel15 and kernel16 helps to cheat clang-format :)

Thanks @mlychkov. Yes, empty line was needed here.

smanna12 · 2020-06-04T20:13:59Z

You seem to have force-pushed again. Please don't.

Sorry for the force-pushed again. Does " git push origin " command work.? It is not allowing me to push the patch.

Do you store new changes into the same commit via amend option? git push doesn't work in this case. You should add new changes into separate commit.

Thanks @mlychkov , yes i used git amend command that's why git push did not work.

bader · 2020-06-06T16:38:41Z

clang/include/clang/Sema/Sema.h

@@ -9917,6 +9917,11 @@ class Sema final {

  bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);

+  // addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute


Suggested change

// addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute

// Adds an intel_reqd_sub_group_size attribute

bader · 2020-06-06T16:43:31Z

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

+// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -verify -pedantic %s
+// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -ast-dump -verify -pedantic %s | FileCheck %s


Doesn't the second command cover both diagnostics and AST checks?

Thanks @bader. Yes, the second command covers both diagnostics and AST checks.
First RUN command is not needed and this is removed now.

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

A little refactoring, addressed to a comment: intel#1807 (comment) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

A little refactoring, addressed to a comment: #1807 (comment) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

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

Template parameter support was added for 1. [[intel::max_global_work_dim)]] attribute on intel#2816 2. [[intel::num_simd_work_items()]] attribute on intel#2510 3. [[intel::reqd_sub_group_size()]] attribute on intel#1807 This patch adds the following new test cases that were not there before to improve the support: 1. Test that checks wrong function template instantiation and ensures that the type is checked properly when instantiating from the template definition. 2. Test that checks expression is not a constant expression. 3. Test that checks expression is a constant expression. 4. Test that checks template parameter support on function. NOTE: No change in compiler. All new test cases have already been supported. Signed-off-by: Soumi Manna <soumi.manna@intel.com>

…3089) Template parameter support was added for 1. [[intel::max_global_work_dim)]] attribute on #2816 2. [[intel::num_simd_work_items()]] attribute on #2510 3. [[intel::reqd_sub_group_size()]] attribute on #1807 This patch adds the following new test cases that were not there before to improve the support: 1. Test that checks wrong function template instantiation and ensures that the type is checked properly when instantiating from the template definition. 2. Test that checks expression is not a constant expression. 3. Test that checks expression is a constant expression. 4. Test that checks template parameter support on function. NOTE: No change in compiler. All new test cases have already been supported. Signed-off-by: Soumi Manna <soumi.manna@intel.com>

Update FileCheck patterns for llvm-project commit eae26b6 ("[IRBuilder] Use canonical i64 type for insertelement index used by vector splats.", 2023-01-11). Original commit: KhronosGroup/SPIRV-LLVM-Translator@ced105f

smanna12 requested review from erichkeane and Fznamznon as code owners June 2, 2020 20:52

smanna12 changed the title ~~[SYCL] Add template parameter support for intel_reqd_sub_group_size a…~~ [Do Not Review] Add template parameter support for intel_reqd_sub_group_size a… Jun 2, 2020

erichkeane reviewed Jun 2, 2020

View reviewed changes

clang/test/SemaOpenCL/invalid-kernel-attrs.cl Show resolved Hide resolved

smanna12 changed the title ~~[Do Not Review] Add template parameter support for intel_reqd_sub_group_size a…~~ [SYCL] Add template parameter support for intel_reqd_sub_group_size a… Jun 2, 2020

smanna12 force-pushed the templateSubAttribute branch 2 times, most recently from 158622c to d5481a6 Compare June 2, 2020 23:43

smanna12 requested a review from premanandrao June 2, 2020 23:47

MrSidims reviewed Jun 3, 2020

View reviewed changes

Fznamznon reviewed Jun 3, 2020

View reviewed changes

smanna12 force-pushed the templateSubAttribute branch 2 times, most recently from a6ec392 to 61a072f Compare June 3, 2020 20:41

[SYCL] Add template parameter support for intel_reqd_sub_group_size a…

d96007d

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 force-pushed the templateSubAttribute branch from 61a072f to d96007d Compare June 4, 2020 02:59

erichkeane reviewed Jun 4, 2020

View reviewed changes

smanna12 added 2 commits June 4, 2020 08:12

[SYCL] Add template parameter support for intel_reqd_sub_group_size a…

60c7b00

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

[SYCL] Add template parameter support for intel_reqd_sub_group_size a…

57d61ce

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

[SYCL] Add template parameter support for intel_reqd_sub_group_size a…

462320e

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 requested review from MrSidims, erichkeane, Fznamznon and mlychkov June 5, 2020 19:02

erichkeane previously approved these changes Jun 5, 2020

View reviewed changes

MrSidims previously approved these changes Jun 5, 2020

View reviewed changes

bader reviewed Jun 6, 2020

View reviewed changes

[SYCL] Add template parameter support for intel_reqd_sub_group_size a…

257f6ee

…ttribute Signed-off-by: Soumi Manna <soumi.manna@intel.com>

smanna12 dismissed stale reviews from MrSidims and erichkeane via 257f6ee June 7, 2020 04:23

bader merged commit 0ae9729 into sycl Jun 7, 2020

bader deleted the templateSubAttribute branch June 7, 2020 16:19

MrSidims added a commit to MrSidims/llvm that referenced this pull request Jun 10, 2020

[NFC][SYCL][FPGA] Code reduction for FPGA memory attributes

8b3a439

A little refactoring, addressed to a comment: intel#1807 (comment) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

MrSidims mentioned this pull request Jun 10, 2020

[NFC][SYCL][FPGA] Code reduction for FPGA memory attributes #1867

Merged

bader pushed a commit that referenced this pull request Jun 11, 2020

[NFC][SYCL][FPGA] Code reduction for FPGA memory attributes (#1867)

03d934e

A little refactoring, addressed to a comment: #1807 (comment) Signed-off-by: Dmitry Sidorov <dmitry.sidorov@intel.com>

AlexeySachkov mentioned this pull request Jun 16, 2020

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

Merged

smanna12 mentioned this pull request Jan 25, 2021

[SYCL] Update tests with new test cases for FPGA function attributes #3089

Merged

	<< Attr.getAttrName() << /non-negative/ 1;
	<< Attr.getAttrName() << /positive/ 0;

	// The SubGroupSize expression is a constant expressions.
	// The SubGroupSize expression is a constant expression.

		@@ -9917,6 +9917,11 @@ class Sema final {

		bool checkNSReturnsRetainedReturnType(SourceLocation loc, QualType type);

		// addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute

	// addIntelReqdSubGroupSizeAttr - Adds an intel_reqd_sub_group_size attribute
	// Adds an intel_reqd_sub_group_size attribute

		// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -verify -pedantic %s
		// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -ast-dump -verify -pedantic %s \| FileCheck %s

[SYCL] Add template parameter support for intel_reqd_sub_group_size a… #1807

[SYCL] Add template parameter support for intel_reqd_sub_group_size a… #1807

Uh oh!

Conversation

smanna12 commented Jun 2, 2020

Uh oh!

Uh oh!

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

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 commented Jun 3, 2020

Uh oh!

smanna12 commented Jun 3, 2020

Uh oh!

erichkeane commented Jun 4, 2020

Uh oh!

smanna12 commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smanna12 commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlychkov commented Jun 4, 2020

Uh oh!

mlychkov commented Jun 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 commented Jun 4, 2020

Uh oh!

bader commented Jun 4, 2020

Uh oh!

smanna12 commented Jun 4, 2020

Uh oh!

smanna12 commented Jun 4, 2020

Uh oh!

smanna12 commented Jun 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader Jun 6, 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!

Uh oh!

smanna12 commented Jun 4, 2020 •

edited

Loading

smanna12 commented Jun 4, 2020 •

edited

Loading

bader Jun 6, 2020 •

edited

Loading