[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D10599: [OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
From: Aaron Ballman via cfe-commits <cfe-commits () lists ! llvm ! org>
Date: 2015-11-06 15:55:27
Message-ID: 6fdeff006f0a239d5882dafa1a26831f () localhost ! localdomain
[Download RAW message or body]
aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/AttrDocs.td:1620
@@ +1619,3 @@
+ let Content = [{
+The declare simd construct can be applied to a function to enable the creation of \
one or more versions that can process multiple arguments using SIMD instructions from \
a single invocation from a SIMD loop. The declare simd directive is a declarative \
directive. There may be multiple declare simd directives for a function. The use of a \
declare simd construct on a function enables the creation of SIMD +versions of the \
associated function that can be used to process multiple arguments from a single \
invocation from a SIMD loop concurrently.
----------------
Line length is an issue here. ;-)
Also, "from a single invocation from a SIMD loop"; should that be "from a single \
invocation of a SIMD loop" instead?
Any time you use "declare simd" as a syntactic phrase, it should be properly \
formatted for RST as ``declare simd``.
================
Comment at: lib/Parse/ParseOpenMP.cpp:97
@@ -89,1 +96,3 @@
+ AccessSpecifier &AS, ParsedAttributesWithRange &Attrs,
+ DeclSpec::TST TagType, Decl *TagDecl) {
assert(Tok.is(tok::annot_pragma_openmp) && "Not an OpenMP directive!");
----------------
Can we not name the last parameter with an identifier that is also a type name? That \
always weirds me out. ;-)
================
Comment at: lib/Parse/ParseOpenMP.cpp:152
@@ +151,3 @@
+ } else {
+ Ptr = ParseCXXClassMemberDeclarationWithPragmas(AS, Attrs, TagType,
+ TagDecl);
----------------
Can you add a test case for a member declaration that has attributes to ensure the \
attributes are properly handled here? In fact, it would be good to add one for the \
top-level function as well.
================
Comment at: lib/Sema/SemaOpenMP.cpp:2370
@@ +2369,3 @@
+
+ auto *NewAttr = new (Context) OMPDeclareSimdDeclAttr(
+ SourceRange(StartLoc, StartLoc), Context, /*SI=*/0);
----------------
Please use OMPDeclareSimdDeclAttr::CreateImplicit instead.
http://reviews.llvm.org/D10599
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic