Page MenuHomePhabricator

[OPENMP 4.0] Initial support for '#pragma omp declare simd' directive.
ClosedPublic

Authored by ABataev on Jun 22 2015, 4:35 AM.

Details

Summary

Initial parsing/sema/serialization/deserialization support for '#pragma omp declare simd' directive.
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.
If the function has any declarations, then the declare simd construct for any declaration that has one must be equivalent to the one specified for the definition. Otherwise, the result is unspecified.
This pragma can be applied many times to the same declaration.
Internally this pragma is represented as an attribute. But we need special processing for this pragma because it must be used before function declaration, this directive is applied to.
A delayed parsing is used for parsing a pragma itself, because this directive has several clauses that can reference arguments of the associated function.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith added inline comments.Jul 22 2015, 1:47 PM
include/clang/AST/ASTMutationListener.h
118–122 ↗(On Diff #29297)

Why do you need this? Isn't the attribute always applied to a "fresh" declaration (one that was recently created, rather than one that was imported from an external source)?

include/clang/Basic/Attr.td
2059 ↗(On Diff #29297)

Comment is out of date.

2070–2071 ↗(On Diff #29297)

Why do you need setters?

include/clang/Parse/Parser.h
1032 ↗(On Diff #29297)

exception-specification?

lib/AST/DeclPrinter.cpp
412–427 ↗(On Diff #29297)

This level of special-casing is not OK, please find a more general way of dealing with this.

lib/Parse/ParseOpenMP.cpp
87 ↗(On Diff #29297)

IsInTagDecl is the wrong name for this; enums are tag decls, but they shouldn't get this treatment.

244 ↗(On Diff #29297)

What does this have to do with "TemplateFunction"s?

268 ↗(On Diff #29297)

This doesn't make sense; that function is for MS-compatibility delayed template parsing, which seems entirely unrelated to this OpenMP pragma.

ABataev updated this revision to Diff 30669.Jul 26 2015, 9:00 PM
ABataev updated this object.
ABataev edited edge metadata.

Update after review

Please find a better way to handle this attribute in DeclPrinter. Perhaps you could generalize your existing support to cover all attributes with pragma spelling?

lib/Parse/ParseDeclCXX.cpp
2816 ↗(On Diff #30669)

Please can you split this refactoring out into a separate commit? That'll make the functional change here (handling annot_pragma_openmp) much easier for future code archaeologists to see.

lib/Parse/ParseOpenMP.cpp
50–73 ↗(On Diff #30669)

Bracing here is now inconsistent.

150 ↗(On Diff #30669)

Add braces around this multi-line else (and the corresponding if for consistency).

154 ↗(On Diff #30669)

Do you really need both these checks?

171–177 ↗(On Diff #30669)

Why are you doing this delayed parsing? You still seem to have no tests that require it.

lib/Sema/SemaOpenMP.cpp
2153 ↗(On Diff #30669)

You don't use Clauses for anything in here; is that intentional?

Richard, thanks for the review. I'll try to fix printing of pragmas.

lib/Parse/ParseDeclCXX.cpp
2816 ↗(On Diff #30669)

Ok, done

lib/Parse/ParseOpenMP.cpp
50–73 ↗(On Diff #30669)

Fixed, thanks

150 ↗(On Diff #30669)

Done, thanks

154 ↗(On Diff #30669)

Removed the second one and turned it to assert()

171–177 ↗(On Diff #30669)

We need it for future parsing of clauses associated with the 'declare simd' construct. Some of the clauses may have references to function arguments. That's why I'm using delayed parsing: at first we need to parse a function along with arguments and only then we'll parse all the pragmas along with their clauses and references to args.
Of course currently it is not used, because this patch does not introduce parsing of clauses. It will be added later.

lib/Sema/SemaOpenMP.cpp
2153 ↗(On Diff #30669)

It will be used later when we'll add parsing of clauses for this pragma

rsmith added inline comments.Jul 31 2015, 2:07 PM
lib/Parse/ParseOpenMP.cpp
171–177 ↗(On Diff #30669)

This code won't work for that; you've left the scope of the function parameters, so lookup for them will fail here. Generally-speaking, we don't like speculative / untested code to be committed, and would prefer to hold back on those changes until we can actually test them in some way.

Here's what I suggest: remove the delayed parsing code from here for this commit (parse the pragma first, then parse the nested declaration, then act on the result), and bring that code back once you introduce parsing for a clause that needs it, if indeed that's the right approach for those clauses. (As I mentioned before, if all they do is to provide a list of identifiers naming parameters, you don't need delay parsing and can instead store them as identifiers and look them up in Sema after the fact. On the other hand, if this pragma allows an arbitrary expression referencing a parameter to appear before the declaration of that parameter, then we'll need something like delayed parsing.)

rsmith added a subscriber: rsmith.Jul 31 2015, 2:54 PM

See also this unanswered question on the OpenMP forums:
http://openmp.org/forum/viewtopic.php?f=12&t=1532

Ok,
I'll remove it for now and rework it later when it will be required.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

01.08.2015 0:07, Richard Smith пишет:

rsmith added inline comments.

================
Comment at: lib/Parse/ParseOpenMP.cpp:171-177
@@ +170,9 @@
+
+ Append the current token at the end of the new token stream so that it
+
doesn't get lost.
+ CachedPragmas.push_back(Tok);
+ Push back tokens for pragma.
+ PP.EnterTokenStream(CachedPragmas.data(), CachedPragmas.size(),
+ /*DisableMacroExpansion=*/true,
+ /*OwnsTokens=*/false);
+
Parse pragma itself.


ABataev wrote:

rsmith wrote:

Why are you doing this delayed parsing? You still seem to have no tests that require it.

We need it for future parsing of clauses associated with the 'declare simd' construct. Some of the clauses may have references to function arguments. That's why I'm using delayed parsing: at first we need to parse a function along with arguments and only then we'll parse all the pragmas along with their clauses and references to args.
Of course currently it is not used, because this patch does not introduce parsing of clauses. It will be added later.

This code won't work for that; you've left the scope of the function parameters, so lookup for them will fail here. Generally-speaking, we don't like speculative / untested code to be committed, and would prefer to hold back on those changes until we can actually test them in some way.

Here's what I suggest: remove the delayed parsing code from here for this commit (parse the pragma first, then parse the nested declaration, then act on the result), and bring that code back once you introduce parsing for a clause that needs it, if indeed that's the right approach for those clauses. (As I mentioned before, if all they do is to provide a list of identifiers naming parameters, you don't need delay parsing and can instead store them as identifiers and look them up in Sema after the fact. On the other hand, if this pragma allows an arbitrary expression referencing a parameter to appear before the declaration of that parameter, then we'll need something like delayed parsing.)

http://reviews.llvm.org/D10599

ABataev updated this revision to Diff 31216.Aug 3 2015, 12:51 AM

Update after review

rsmith added inline comments.Aug 25 2015, 7:04 PM
include/clang/Basic/Attr.td
2074–2078 ↗(On Diff #31216)

What's this for?

include/clang/Basic/DiagnosticParseKinds.td
996–999 ↗(On Diff #31216)

with -> after, in both diagnstics.

include/clang/Basic/DiagnosticSemaKinds.td
7661 ↗(On Diff #31216)

can be applied to functions only -> can only be applied to functions

lib/AST/DeclPrinter.cpp
214 ↗(On Diff #31216)

It looks like we would no longer print attributes with pragma spelling for non-function declarations after this change (but I don't think we have any such pragmas at the moment). I don't want to ask you to write a bunch more code here that you can't test, but can you add a FIXME somewhere in here to add calls to print pragmas in more places?

lib/Parse/ParseOpenMP.cpp
133 ↗(On Diff #31216)

You should bail out if you don't actually find your desired token. If you carry on without checking, your ConsumeToken call might consume the tok::eof token or some similar bad thing.

144–145 ↗(On Diff #31216)

I find this loop slightly surprising: the cases where Parse*Declaration return a null DeclGroupPtrTy are when they parsed some declaration-like entity (such as a declaration-like pragma) but didn't create some declaration to represent it.

Do you really want to allow such things between your pragma and its associated function? As an extreme case, you'd accept things like this:

#pragma omp declare simd
#pragma pack (...)
;
public:
__if_exists(foo) {
  int n;
}
int the_simd_function();
145 ↗(On Diff #31216)

Do you really need to check both !Ptr and Ptr.get().isNull() here? Generally, the parser shouldn't really be calling get() on an OpaquePtr -- these pointers are supposed to be opaque to the parser.

160 ↗(On Diff #31216)

The only way you could get here without being at EOF or EOM would be if you hit an unexpected right-brace. Shouldn't you diagnose that? For instance, it looks like you won't diagnose this case:

namespace N {
  #pragma omp declare simd
}
162–168 ↗(On Diff #31216)

I would suggest that you instead call ActOnOpenMPDeclareSimdDirective unconditionally, pass in Ptr rather than its contained declaration, and check for a single declaration in Sema -- this seems much more of a semantic check than a syntactic one.

ABataev marked 7 inline comments as done.Aug 25 2015, 11:04 PM

Richard, thanks for the review!

include/clang/Basic/Attr.td
2074–2078 ↗(On Diff #31216)

This is required for proper AST printing of pragma. Without this additional member (this one is required for pragma attributes by attribute generation system) all pragmas will be printed in one line.

include/clang/Basic/DiagnosticParseKinds.td
996–999 ↗(On Diff #31216)

Done, thanks.

include/clang/Basic/DiagnosticSemaKinds.td
7661 ↗(On Diff #31216)

Done

lib/AST/DeclPrinter.cpp
214 ↗(On Diff #31216)

Ok, will do.

lib/Parse/ParseOpenMP.cpp
133 ↗(On Diff #31216)

Done, thanks.

144–145 ↗(On Diff #31216)

We need to be able to combine 'declare simd' pragma with other pragmas, for example:

#pragma omp declare simd
#pragma GCC visibility push(default)
#pragma options align=packed
#pragma omp declare simd
#pragma GCC visibility pop
int tadd(int b) { return x[b] + b; }

We should be able to accept such code. But I will reduce the number of allowed constructs.

145 ↗(On Diff #31216)

Ok, removed Ptr.get().isNull()

162–168 ↗(On Diff #31216)

Ok, done

ABataev updated this revision to Diff 33181.Aug 25 2015, 11:05 PM
ABataev marked 7 inline comments as done.

Update after review

Richard, any comments about latest version?

rsmith added inline comments.Oct 7 2015, 6:25 PM
include/clang/Basic/Attr.td
2098–2102 ↗(On Diff #33181)

Can we instead handle this in the generated code? Presumably we'd get this wrong in the same way for all attributes with pragma spelling?

lib/Parse/ParseOpenMP.cpp
133 ↗(On Diff #33181)

Hmm, on reflection it would be better to use:

while (Tok.isNot(tok::annot_pragma_openmp_end))
  ConsumeAnyToken();

We know that there is an annot_pragma_openmp_end token coming, but SkipUntil might stop early if there's (say) a stray right-paren in the token stream.

144–145 ↗(On Diff #33181)

I don't think your example is reasonable to accept. Right now (prior to your patch), we have two fundamentally different kinds of pragmas:

  1. Lexer-level pragmas. These can appear anywhere in the token stream, and take effect immediately. These are handled entirely by the lexer.
  1. Declaration-like pragmas. These can only appear where a declaration is permitted, and act as if they declare ... something. We transform these into tokens and handle them in the parser.

You're adding a third kind of pragma, an attribute-like pragma, that can only appear before declarations. As such, if your pragma appears before a pragma of kind 2 (such as #pragma GCC visibility), it should act as an attribute applying to *that* declaration-like entity.

So I think the only thing you should allow between your pragma and the declaration to which it applies is more attribute-like pragmas.

ABataev marked 3 inline comments as done.Oct 13 2015, 1:21 AM
ABataev added inline comments.
include/clang/Basic/Attr.td
2098–2102 ↗(On Diff #33181)

Done.

lib/Parse/ParseOpenMP.cpp
133 ↗(On Diff #33181)

Done, thanks.

144–145 ↗(On Diff #33181)

Reworked.

ABataev updated this revision to Diff 37218.Oct 13 2015, 1:21 AM
ABataev marked 3 inline comments as done.

Update after review

aaron.ballman added inline comments.Nov 6 2015, 7:55 AM
include/clang/Basic/AttrDocs.td
1620 ↗(On Diff #37218)

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`.

lib/Parse/ParseOpenMP.cpp
97 ↗(On Diff #37218)

Can we not name the last parameter with an identifier that is also a type name? That always weirds me out. ;-)

152 ↗(On Diff #37218)

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.

lib/Sema/SemaOpenMP.cpp
2370 ↗(On Diff #37218)

Please use OMPDeclareSimdDeclAttr::CreateImplicit instead.

ABataev marked 4 inline comments as done.Nov 16 2015, 1:49 AM
ABataev added inline comments.
include/clang/Basic/AttrDocs.td
1620 ↗(On Diff #37218)

Fixed, thanks

lib/Parse/ParseOpenMP.cpp
97 ↗(On Diff #37218)

Fixed, thanks.

152 ↗(On Diff #37218)

Ok, will add

lib/Sema/SemaOpenMP.cpp
2370 ↗(On Diff #37218)

Fixed, thanks

ABataev updated this revision to Diff 40255.Nov 16 2015, 1:50 AM
ABataev marked 4 inline comments as done.

Update after review

aaron.ballman accepted this revision.Nov 16 2015, 12:57 PM
aaron.ballman edited edge metadata.

I don't think we generally want files commit with explicit svn properties like your new test files have. However, beyond that, LGTM. You should still wait for approval from Richard before committing, however.

This revision is now accepted and ready to land.Nov 16 2015, 12:57 PM

Richard, any comments?

rjmccall edited edge metadata.Dec 16 2015, 9:46 AM

Richard should really be the one to sign off on this, since he's been managing the review so far.

I am curious why we decided to handle this separately instead of treating it as a different attribute spelling kind.

Richard should really be the one to sign off on this, since he's been managing the review so far.

I am curious why we decided to handle this separately instead of treating it as a different attribute spelling kind.

It is a separate pragma with many clauses (that will be implemented as soon as the pragma itself is committed), so we just can't treat it as a different spelling kind.

ABataev updated this revision to Diff 44833.Jan 13 2016, 10:00 PM
ABataev edited edge metadata.
ABataev removed a subscriber: rsmith.

Updated to latest version

This revision was automatically updated to reflect the committed changes.