Skip to content

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bader merged 6 commits into from
Jun 23, 2020
Merged

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a35cf0
[SYCL] Fix check for reqd_sub_group_size attribute mismatches
AlexeySachkov Jun 16, 2020
608ee3a
[SYCL] Introduce IntegerExprArgument
AlexeySachkov Jun 17, 2020
6b578a6
Fix formatting issue
AlexeySachkov Jun 19, 2020
3bee3df
[SYCL] Revert all improvements, just fix a bug
AlexeySachkov Jun 19, 2020
540853f
[SYCL] Revert change of getIntExprValue return type
AlexeySachkov Jun 22, 2020
c625a69
Update clang/lib/Sema/SemaSYCL.cpp
AlexeySachkov Jun 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ class IdentifierArgument<string name, bit opt = 0> : Argument<name, opt>;
class IntArgument<string name, bit opt = 0> : Argument<name, opt>;
class StringArgument<string name, bit opt = 0> : Argument<name, opt>;
class ExprArgument<string name, bit opt = 0> : Argument<name, opt>;
class IntegerExprArgument<string name, int undef = 0, bit opt = 0>
: ExprArgument<name, opt> {
int UndefValue = undef;
}
class DeclArgument<DeclNode kind, string name, bit opt = 0, bit fake = 0>
: Argument<name, opt, fake> {
DeclNode Kind = kind;
Expand Down Expand Up @@ -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>];
Copy link
Contributor

@MrSidims MrSidims Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let Subjects = SubjectList<[Function, CXXMethod], ErrorDiag>;
let Documentation = [IntelReqdSubGroupSizeDocs];
let LangOpts = [OpenCL, SYCLIsDevice, SYCLIsHost];
Expand Down
12 changes: 3 additions & 9 deletions clang/lib/CodeGen/CodeGenFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,16 +565,10 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
Fn->setMetadata("reqd_work_group_size", llvm::MDNode::get(Context, AttrMDArgs));
}

if (const IntelReqdSubGroupSizeAttr *A =
FD->getAttr<IntelReqdSubGroupSizeAttr>()) {
llvm::APSInt ArgVal(32);
if (IntelReqdSubGroupSizeAttr *A = FD->getAttr<IntelReqdSubGroupSizeAttr>()) {
llvm::LLVMContext &Context = getLLVMContext();
bool IsValid = A->getSubGroupSize()->isIntegerConstantExpr(
ArgVal, FD->getASTContext());
assert(IsValid && "Not an integer constant expression");
(void)IsValid;
llvm::Metadata *AttrMDArgs[] = {
llvm::ConstantAsMetadata::get(Builder.getInt32(ArgVal.getSExtValue()))};
llvm::Metadata *AttrMDArgs[] = {llvm::ConstantAsMetadata::get(
Builder.getInt32(A->getSubGroupSizeEvaluated(FD->getASTContext())))};
Fn->setMetadata("intel_reqd_sub_group_size",
llvm::MDNode::get(Context, AttrMDArgs));
}
Expand Down
13 changes: 3 additions & 10 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@MrSidims MrSidims Jun 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Fznamznon Fznamznon Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use this function then?

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

llvm::APSInt Val(32);
bool IsValid = E->isIntegerConstantExpr(Val, S.getASTContext());
assert(IsValid && "expression must be constant integer");
return Val.getSExtValue();
}

class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
public:
MarkDeviceFunction(Sema &S)
Expand Down Expand Up @@ -1696,15 +1688,16 @@ void Sema::MarkDevice(void) {
KernelBody ? KernelBody->getAttr<SYCLSimdAttr>() : nullptr;
if (auto *Existing =
SYCLKernel->getAttr<IntelReqdSubGroupSizeAttr>()) {
if (Existing->getSubGroupSize() != Attr->getSubGroupSize()) {
if (Existing->getSubGroupSizeEvaluated(getASTContext()) !=
Attr->getSubGroupSizeEvaluated(getASTContext())) {
Diag(SYCLKernel->getLocation(),
diag::err_conflicting_sycl_kernel_attributes);
Diag(Existing->getLocation(), diag::note_conflicting_attribute);
Diag(Attr->getLocation(), diag::note_conflicting_attribute);
SYCLKernel->setInvalidDecl();
}
} else if (KBSimdAttr &&
(getIntExprValue(*this, Attr->getSubGroupSize()) != 1)) {
(Attr->getSubGroupSizeEvaluated(getASTContext()) != 1)) {
reportConflictingAttrs(*this, KernelBody, KBSimdAttr, Attr);
} else {
SYCLKernel->addAttr(A);
Expand Down
9 changes: 9 additions & 0 deletions clang/test/SemaSYCL/reqd-sub-group-size-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,16 @@ void bar() {
baz();
});
#endif

kernel<class kernel_name5>([]() [[cl::intel_reqd_sub_group_size(2)]] { });
kernel<class kernel_name6>([]() [[cl::intel_reqd_sub_group_size(4)]] { foo(); });
}

[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B();
[[cl::intel_reqd_sub_group_size(16)]] void A() {
}
[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B() {
A();
}

#ifdef TRIGGER_ERROR
Expand Down
32 changes: 32 additions & 0 deletions clang/utils/TableGen/ClangAttrEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,6 +1161,36 @@ namespace {
void writeHasChildren(raw_ostream &OS) const override { OS << "true"; }
};

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 << " }";
}
};

Copy link
Contributor

@Fznamznon Fznamznon Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@erichkeane erichkeane Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@erichkeane erichkeane Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@MrSidims MrSidims Jun 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@MrSidims MrSidims Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Reverted all my experiments in 3bee3df

class VariadicExprArgument : public VariadicArgument {
public:
VariadicExprArgument(const Record &Arg, StringRef Attr)
Expand Down Expand Up @@ -1299,6 +1329,8 @@ createArgument(const Record &Arg, StringRef Attr,
Ptr = std::make_unique<EnumArgument>(Arg, Attr);
else if (ArgName == "ExprArgument")
Ptr = std::make_unique<ExprArgument>(Arg, Attr);
else if (ArgName == "IntegerExprArgument")
Ptr = std::make_unique<IntegerExprArgument>(Arg, Attr);
else if (ArgName == "DeclArgument")
Ptr = std::make_unique<SimpleArgument>(
Arg, Attr, (Arg.getValueAsDef("Kind")->getName() + "Decl *").str());
Expand Down