aaron.ballman created this revision.
aaron.ballman added a reviewer: Anastasia.
Herald added subscribers: zzheng, yaxunl.
aaron.ballman requested review of this revision.
Herald added a project: clang.
While working on a downstream project, I noticed some issues with the
`opencl_unroll_hint` implementation. The attribute definition claimed the
attribute was inheritable (which only applies to declaration attributes) and
not a statement attribute. Further, it treats subject appertainment errors as
being parse errors rather than semantic errors, which leads to us accepting
invalid code. For instance, we currently fail to reject:
void foo() {
int i = 1000;
__attribute__((nomerge, opencl_unroll_hint(8)))
if (i) { foo(); }
}
This patch address the issues by clarifying that `opencl_unroll_hint` is a
statement attribute and handling its appertainment checks in the semantic layer
instead of the parsing layer. This changes the output of the diagnostic text to
be more consistent with other appertainment errors.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D96043
Files:
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/include/clang/Parse/Parser.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/test/Parser/opencl-unroll-hint.cl
Index: clang/test/Parser/opencl-unroll-hint.cl
===================================================================
--- clang/test/Parser/opencl-unroll-hint.cl
+++ clang/test/Parser/opencl-unroll-hint.cl
@@ -1,8 +1,21 @@
-//RUN: %clang_cc1 -O0 -cl-std=CL2.0 -fsyntax-only -verify %s
+//RUN: %clang_cc1 -cl-std=CL2.0 -fsyntax-only -verify %s
kernel void B (global int *x) {
- __attribute__((opencl_unroll_hint(42)))
- if (x[0]) // expected-error {{OpenCL only supports 'opencl_unroll_hint' attribute on for, while, and do statements}}
+ __attribute__((opencl_unroll_hint(42))) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+ if (x[0])
x[0] = 15;
}
+void parse_order_error() {
+ // Ensure we properly diagnose OpenCL loop attributes on the incorrect
+ // subject in the presence of other attributes.
+ int i = 1000;
+ __attribute__((nomerge, opencl_unroll_hint(8))) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+ if (i) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+
+ __attribute__((opencl_unroll_hint(8), nomerge)) // expected-error {{'opencl_unroll_hint' attribute only applies to 'for', 'while', and 'do' statements}}
+ if (i) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+
+ __attribute__((nomerge, opencl_unroll_hint(8))) // OK
+ while (1) { parse_order_error(); } // Recursive call silences unrelated diagnostic about nomerge.
+}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===================================================================
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -378,6 +378,12 @@
// determines unrolling factor) or 1 argument (the unroll factor provided
// by the user).
+ if (!isa<ForStmt, CXXForRangeStmt, DoStmt, WhileStmt>(St)) {
+ S.Diag(A.getLoc(), diag::err_attribute_wrong_decl_type_str)
+ << A << "'for', 'while', and 'do' statements";
+ return nullptr;
+ }
+
unsigned NumArgs = A.getNumArgs();
if (NumArgs > 1) {
Index: clang/lib/Parse/ParseStmt.cpp
===================================================================
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -98,10 +98,15 @@
ParenBraceBracketBalancer BalancerRAIIObj(*this);
+ // Because we're parsing either a statement or a declaration, the order of
+ // attribute parsing is important. [[]] attributes at the start of a
+ // statement are different from [[]] attributes that follow an __attribute__
+ // at the start of the statement. Thus, we're not using MaybeParseAttributes
+ // here because we don't want to allow arbitrary orderings.
ParsedAttributesWithRange Attrs(AttrFactory);
MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true);
- if (!MaybeParseOpenCLUnrollHintAttribute(Attrs))
- return StmtError();
+ if (getLangOpts().OpenCL)
+ MaybeParseGNUAttributes(Attrs);
StmtResult Res = ParseStatementOrDeclarationAfterAttributes(
Stmts, StmtCtx, TrailingElseLoc, Attrs);
@@ -2548,19 +2553,3 @@
}
Braces.consumeClose();
}
-
-bool Parser::ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
- MaybeParseGNUAttributes(Attrs);
-
- if (Attrs.empty())
- return true;
-
- if (Attrs.begin()->getKind() != ParsedAttr::AT_OpenCLUnrollHint)
- return true;
-
- if (!(Tok.is(tok::kw_for) || Tok.is(tok::kw_while) || Tok.is(tok::kw_do))) {
- Diag(Tok, diag::err_opencl_unroll_hint_on_non_loop);
- return false;
- }
- return true;
-}
Index: clang/include/clang/Parse/Parser.h
===================================================================
--- clang/include/clang/Parse/Parser.h
+++ clang/include/clang/Parse/Parser.h
@@ -2818,17 +2818,6 @@
void ParseBorlandTypeAttributes(ParsedAttributes &attrs);
void ParseOpenCLKernelAttributes(ParsedAttributes &attrs);
void ParseOpenCLQualifiers(ParsedAttributes &Attrs);
- /// Parses opencl_unroll_hint attribute if language is OpenCL v2.0
- /// or higher.
- /// \return false if error happens.
- bool MaybeParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs) {
- if (getLangOpts().OpenCL)
- return ParseOpenCLUnrollHintAttribute(Attrs);
- return true;
- }
- /// Parses opencl_unroll_hint attribute.
- /// \return false if error happens.
- bool ParseOpenCLUnrollHintAttribute(ParsedAttributes &Attrs);
void ParseNullabilityTypeSpecifiers(ParsedAttributes &attrs);
VersionTuple ParseVersionTuple(SourceRange &Range);
Index: clang/include/clang/Basic/DiagnosticParseKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticParseKinds.td
+++ clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1220,9 +1220,6 @@
"omit the namespace to add attributes to the most-recently"
" pushed attribute group">;
-def err_opencl_unroll_hint_on_non_loop : Error<
- "OpenCL only supports 'opencl_unroll_hint' attribute on for, while, and do statements">;
-
// OpenCL EXTENSION pragma (OpenCL 1.1 [9.1])
def warn_pragma_expected_colon : Warning<
"missing ':' after %0 - ignoring">, InGroup<IgnoredPragmas>;
Index: clang/include/clang/Basic/Attr.td
===================================================================
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -1178,8 +1178,10 @@
let SimpleHandler = 1;
}
-def OpenCLUnrollHint : InheritableAttr {
+def OpenCLUnrollHint : StmtAttr {
let Spellings = [GNU<"opencl_unroll_hint">];
+// let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+// ErrorDiag, "'for', 'while', and 'do' statements">;
let Args = [UnsignedArgument<"UnrollHint">];
let Documentation = [OpenCLUnrollHintDocs];
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
