[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

AlexeySachkov · 2020-06-16T17:00:25Z

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

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

AlexeySachkov · 2020-06-16T17:01:43Z

clang/lib/Sema/SemaSYCL.cpp

@@ -308,14 +308,6 @@ static void reportConflictingAttrs(Sema &S, FunctionDecl *F, const Attr *A1,
  F->setInvalidDecl();
 }

-// Returns the signed constant integer value represented by given expression.
-static int64_t getIntExprValue(Sema &S, const Expr *E) {


I decided to replace this function with extension to IntelReqdSubGroupSizeAttr class to add caching functionality to avoid re-evaluating argument expression each time we need the value, but I'm not entirely sure that this is a correct approach or I implemented it correctly - please advise here

I have a feeling, that I've already seen that type of function is Sema, like 'check unsigned int arguments' (can't grep it right now), so I see no value in the function above.

Should we use this function then?

Should we use this function then?

I prefer to have some cached value instead of evaluating attribute argument each time we need it to perform some correctness check

MrSidims · 2020-06-16T17:16:38Z

clang/include/clang/Basic/Attr.td

+    int SubGroupSize = 0;
+
+  public:
+    int getSubGroupSizeEvaluated(ASTContext &Ctx) {


That is an interesting approach. I was just about refactoring FPGA specific attributes and may adopt it.

Do you consider if it's possible/worth to move this function (with renaming) to InheritableAttr or even create a new class, like SYCLFunctionAttr?

I mean, we have tons of attributes with similar routines.

Do you consider if it's possible/worth to move this function (with renaming) to InheritableAttr or even create a new class, like SYCLFunctionAttr?

Good point, I will take a look at FPGA-related attributes and propose something more common/generic in this PR

It's not necessary to do it right now - currently FPGA attributes doesn't support template instantiation of arguments (add support for it is the main purpose of the patch I'm preparing).

@MrSidims, see 608ee3a - I've introduced new type of attribute argument, which should be re-usable by others as well.

add support for it is the main purpose of the patch I'm preparing

Could you please try to embed my PR into your patch to see if proposed way can actually be re-used by someone else and works fine?

Just tried it out. It works perfectly well with num_simd_work_items attribute.

New attribute argument class is intended to provide functionality to evaluate and cache value of ExprArgument which holds an integer value Updated IntelReqdSubGroupSizeAttr to use new argument type

MrSidims

Other than 'clang-format' issue the patch is LGTM

MrSidims · 2020-06-18T15:41:57Z

clang/include/clang/Basic/Attr.td

@@ -1289,7 +1293,7 @@ def LoopUnrollHint : InheritableAttr {

 def IntelReqdSubGroupSize: InheritableAttr {
  let Spellings = [GNU<"intel_reqd_sub_group_size">, CXX11<"cl", "intel_reqd_sub_group_size">];
-  let Args = [ExprArgument<"SubGroupSize">];
+  let Args = [IntegerExprArgument<"SubGroupSize", /* UndefValue = */ 0>];


I've started thinking, that 'named' arguments, even if they are more readable, are actually not good. By naming arguments as faceless 'Value' we can unify handlers for many attributes. But I'm doing renaming in a patch for FPGA attributes, no need to bring it here.

Fznamznon · 2020-06-19T08:38:50Z

clang/utils/TableGen/ClangAttrEmitter.cpp

+  class IntegerExprArgument : public ExprArgument {
+  private:
+    int UndefValue;
+
+  public:
+    IntegerExprArgument(const Record &Arg, StringRef Attr)
+        : ExprArgument(Arg, Attr), UndefValue(Arg.getValueAsInt("UndefValue")) {
+    }
+
+    void writeAccessors(raw_ostream &OS) const override {
+      OS << "  " << getType() << " get" << getUpperName() << "() const {\n";
+      OS << "    return " << getLowerName() << ";\n";
+      OS << "  }\n";
+      OS << "private:\n";
+      OS << "  int " << getLowerName() << "Value = " << UndefValue << ";\n";
+      OS << "public:\n";
+      OS << "  int get" << getUpperName() << "Evaluated(ASTContext &Ctx) {\n";
+      OS << "    if (" << getLowerName() << "Value != " << UndefValue << ")\n";
+      OS << "      return " << getLowerName() << "Value;\n";
+      OS << "    llvm::APSInt Val(32);\n";
+      OS << "    bool Succeedded = " << getLowerName()
+         << "->isIntegerConstantExpr(Val, Ctx);\n";
+      OS << "    assert(Succeedded && \"expression must be constant "
+            "integer\");\n";
+      OS << "    " << getLowerName() << "Value = Val.getSExtValue();\n";
+      OS << "    return " << getLowerName() << "Value;\n";
+      OS << "  }";
+    }
+  };
+


Wow, I haven't seen such stuff before.
@erichkeane , could you please review and comment on approach?

I don't think this is the right approach, we shouldn't be adding helper functions to the tablegen types like this. Typically we use these only when we need to represent new argument types.

While I get the wish to have a 'cached' value, evaluating the expression only happens a handful of times. Additionally, the effort to replace the constant-evaluator includes a 'memory' of evaluated values. When this

If we REALLY need it short term, we generally put a helper function into AdditionalMembers.

While I get the wish to have a 'cached' value, evaluating the expression only happens a handful of times.

If I understand correctly, it can happen a lot of times and actually depends on how code is written. Basically we compare IntelReqdSubGroupSizeAttr object which attached to a kernel function with each IntelReqdSubGroupSizeAttr object attached to any callee (or callee of callee - we analyze the whole call graph) of the kernel. Some applications just put this attribute into a macro and attach it to almost every single function, which means significant amount of evaluations

Additionally, the effort to replace the constant-evaluator includes a 'memory' of evaluated values. When this

Seems like unfinished sentence

If we REALLY need it short term, we generally put a helper function into AdditionalMembers.

This is what I did originally, but then @MrSidims said that he needs similar thing for several other attributes and I tried to make something generic and re-usable here

I don't think this is the right approach, we shouldn't be adding helper functions to the tablegen types like this. Typically we use these only when we need to represent new argument types.

I'm open to other suggestions, right now I see two other ways:

Just a helper function somewhere to evaluate Expr *, something like this. No caching, no modifications to tablegen

Common class for our attributes with Expr arguments, which declares AdditionalMembers - we will still be able to re-use it for other attributes, but we will have to get rid of "named" attribute arguments to do so (this comment from @MrSidims)

While I get the wish to have a 'cached' value, evaluating the expression only happens a handful of times.

If I understand correctly, it can happen a lot of times and actually depends on how code is written. Basically we compare IntelReqdSubGroupSizeAttr object which attached to a kernel function with each IntelReqdSubGroupSizeAttr object attached to any callee (or callee of callee - we analyze the whole call graph) of the kernel. Some applications just put this attribute into a macro and attach it to almost every single function, which means significant amount of evaluations

Potentially, but it also depends on the cost of the expression. Is anyone using massive constexpr functions to generate these? if it is just a Constant integer or something (or a sizeof/simple expression), the cost is still minimal.

Additionally, the effort to replace the constant-evaluator includes a 'memory' of evaluated values. When this

Seems like unfinished sentence

Yeah, I meant to say, when that happens, this isn't going to be necessary.

If we REALLY need it short term, we generally put a helper function into AdditionalMembers.

This is what I did originally, but then @MrSidims said that he needs similar thing for several other attributes and I tried to make something generic and re-usable here

I don't think this is the right approach, we shouldn't be adding helper functions to the tablegen types like this. Typically we use these only when we need to represent new argument types.

I'm open to other suggestions, right now I see two other ways:

* Just a helper function somewhere to evaluate `Expr *`, something like [this](https://github.com/intel/llvm/pull/1905/commits/8a35cf0a2a8643599bc3a10d65bedb28963d7b80#diff-c88a590aca6d5f8e92dd1f8b6ec05a49L312). No caching, no modifications to tablegen

Something like that seems to be fine to me. If we want to cache an Expr* to int-result relationship, we can do so there. We do something similar (using a densemap) for record size vs recorddecl, so a similar solution here seems right.

* Common class for our attributes with Expr arguments, which declares `AdditionalMembers` - we will still be able to re-use it for other attributes, but we will have to get rid of "named" attribute arguments to do so ([this comment](https://github.com/intel/llvm/pull/1905#discussion_r442322546) from @MrSidims)

I don't think we want to remove named arguments. That doesn't seem like a good idea. And I don't really see how it solves this problem.

Potentially, but it also depends on the cost of the expression. Is anyone using massive constexpr functions to generate these? if it is just a Constant integer or something (or a sizeof/simple expression), the cost is still minimal.

I'm not sure about massive constexpr stuff, most likely this is just a template argument or something like this

Thanks for the feedback. I will then probably revert everything back to a single helper function for integer evaluation (and check if something like this already exists and we could re-use that)

Lets fix the bug first and then we can discuss, what refactoring we want/can do.

I don't think we want to remove named arguments. That doesn't seem like a good idea. And I don't really see how it solves this problem.

For example we have two attributes reqd_sub_group_size and num_simd_work_items. Both of them are single argument of positive integer attributes. To handle them we have two functions: addReqdSubGorupSizeAttr (which calls getReqdSubGroupSize) and addNumSimdWorkItemsAttr (which calls getNumSimd). Since both of the attributes shares the same semantic, we can replace these two functions with addSingleArgPositiveIntAttr (which shall call getValue). And later on we can reuse this function instead of writing tons of copy-paste code for every new attribute.

Well, actually, addSomeAttr not a candidate for re-usability here, instead instantiateSYCLOneArgFunctionAttr is.

Lets fix the bug first and then we can discuss, what refactoring we want/can do.

Reverted all my experiments in 3bee3df

clang/lib/Sema/SemaSYCL.cpp

Co-authored-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

smanna12

Thanks @AlexeySachkov for fixing the mismatch check.

* upstream/sycl: [SYCL] Implement braced-init-list or a number as range for queue::parallel_for (intel#1931) [SYCL][Doc] Add SYCL_INTEL_accessor_properties extension specification (intel#1925) [SYCL-PTX] Add builtins for the relational category (intel#1831) [SYCL][CUDA] Remove unnecessary memfence (intel#1935) [SYCL] Add handling for wrapped sampler (intel#1942) [SYCL] Release notes for June'20 DPCPP implementation update (intel#1948) [SYCL] Fix assert when calling get_binaries() on host (intel#1944) [SYCL] Fix check for reqd_sub_group_size attribute mismatches (intel#1905)

AlexeySachkov requested a review from smanna12 June 16, 2020 17:00

AlexeySachkov requested review from elizabethandrews, Fznamznon and premanandrao as code owners June 16, 2020 17:00

AlexeySachkov commented Jun 16, 2020

View reviewed changes

MrSidims reviewed Jun 16, 2020

View reviewed changes

[SYCL] Introduce IntegerExprArgument

608ee3a

New attribute argument class is intended to provide functionality to evaluate and cache value of ExprArgument which holds an integer value Updated IntelReqdSubGroupSizeAttr to use new argument type

MrSidims previously approved these changes Jun 18, 2020

View reviewed changes

Fix formatting issue

6b578a6

AlexeySachkov dismissed MrSidims’s stale review via 6b578a6 June 19, 2020 08:29

Fznamznon reviewed Jun 19, 2020

View reviewed changes

Fznamznon requested a review from erichkeane June 19, 2020 08:39

AlexeySachkov added 2 commits June 22, 2020 17:07

[SYCL] Revert all improvements, just fix a bug

3bee3df

[SYCL] Revert change of getIntExprValue return type

540853f

Fznamznon reviewed Jun 22, 2020

View reviewed changes

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

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

Update clang/lib/Sema/SemaSYCL.cpp

c625a69

Co-authored-by: Mariya Podchishchaeva <mariya.podchishchaeva@intel.com>

Fznamznon approved these changes Jun 22, 2020

View reviewed changes

smanna12 approved these changes Jun 23, 2020

View reviewed changes

bader merged commit 75b3dc2 into intel:sycl Jun 23, 2020

AlexeySachkov deleted the private/asachkov/fix-reqd-subgroup-size-diags branch February 25, 2021 12:26

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

Uh oh!

Conversation

AlexeySachkov commented Jun 16, 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!

Choose a reason for hiding this comment

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fznamznon Jun 19, 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!

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!

MrSidims Jun 19, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov commented Jun 16, 2020 •

edited

Loading

Fznamznon Jun 19, 2020 •

edited

Loading

MrSidims Jun 19, 2020 •

edited

Loading