[PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 5 17:52:36 PDT 2015
rsmith added inline comments.
================
Comment at: include/clang/AST/DeclOpenMP.h:95
@@ +94,3 @@
+/// #pragma omp declare reduction (foo : int,float : omp_out += omp_in)
+/// initializer (omp_priv = 0)
+/// \endcode
----------------
I think automatic formatting has messed up your example. Maybe indent this line a bit more to show it's a continuation of the previous line.
================
Comment at: include/clang/AST/DeclOpenMP.h:99
@@ +98,3 @@
+/// Here 'omp_out += omp_in' is a combiner and 'omp_priv = 0' is an initializer.
+class OMPDeclareReductionDecl : public DeclaratorDecl, public DeclContext {
+private:
----------------
OK, then you need to update some parts of class `DeclContext` for this. At least the comment on that class needs to be updated, and possibly other parts too.
================
Comment at: include/clang/AST/DeclOpenMP.h:99
@@ +98,3 @@
+/// Here 'omp_out += omp_in' is a combiner and 'omp_priv = 0' is an initializer.
+class OMPDeclareReductionDecl : public DeclaratorDecl, public DeclContext {
+private:
----------------
rsmith wrote:
> OK, then you need to update some parts of class `DeclContext` for this. At least the comment on that class needs to be updated, and possibly other parts too.
Why is this a `DeclaratorDecl` rather than merely a `NamedDecl`? It doesn't have a declarator, or even a type.
================
Comment at: include/clang/AST/DeclOpenMP.h:103-105
@@ +102,5 @@
+ /// \brief Combiner for declare reduction construct.
+ Stmt *Combiner;
+ /// \brief Initializer for declare reduction construct.
+ Stmt *Initializer;
+ /// \brief Reference to the previous declare reduction construct in the same
----------------
Store these as `Expr*`s. We use `Stmt*` arrays in `Expr` nodes to support `StmtIterator`, which is not a concern here.
================
Comment at: include/clang/AST/DeclOpenMP.h:106
@@ +105,3 @@
+ Stmt *Initializer;
+ /// \brief Reference to the previous declare reduction construct in the same
+ /// scope with the same name. Required for proper templates instantiation if
----------------
The comment doesn't match the name. Does this point to the next or previous declaration? The previous decl would make more sense, since AST nodes are intended to be immutable once created. Storing them in this order will also create problems for template instantiation.
================
Comment at: include/clang/AST/DeclOpenMP.h:109
@@ +108,3 @@
+ /// the declare reduction construct is declared inside compound statement.
+ Decl *NextDeclInScope;
+
----------------
Store this as an `OMPDeclareReductionDecl*`.
================
Comment at: include/clang/AST/DeclOpenMP.h:130
@@ +129,3 @@
+ /// \brief Get combiner expression of the declare reduction construct.
+ Expr *getCombiner() { return cast<Expr>(Combiner); }
+ Expr *getCombiner() const { return cast<Expr>(Combiner); }
----------------
This is redundant with the next function. If you keep both, this should return `const Expr*`.
================
Comment at: include/clang/AST/DeclOpenMP.h:137
@@ +136,3 @@
+ /// construct.
+ Expr *getInitializer() { return cast_or_null<Expr>(Initializer); }
+ Expr *getInitializer() const { return cast_or_null<Expr>(Initializer); }
----------------
Likewise.
================
Comment at: include/clang/AST/DeclOpenMP.h:144-146
@@ +143,5 @@
+ /// scope with the same name.
+ OMPDeclareReductionDecl *getNextDeclInScope() {
+ return cast_or_null<OMPDeclareReductionDecl>(NextDeclInScope);
+ }
+ OMPDeclareReductionDecl *getNextDeclInScope() const {
----------------
Likewise.
================
Comment at: lib/AST/ASTContext.cpp:8321-8322
@@ -8320,4 +8320,4 @@
return false;
- } else if (isa<OMPThreadPrivateDecl>(D))
+ } else if (isa<OMPThreadPrivateDecl>(D) || isa<OMPDeclareReductionDecl>(D))
return true;
else
----------------
Can these be forward-declared / used from a different translation unit than their definition? If not, it would seem better to emit them on use rather than emitting them eagerly.
================
Comment at: lib/AST/ItaniumMangle.cpp:70-73
@@ -68,4 +69,6 @@
const DeclContext *DC = D->getDeclContext();
if (const CapturedDecl *CD = dyn_cast<CapturedDecl>(DC))
return getEffectiveDeclContext(CD);
+ if (auto *DR = dyn_cast<OMPDeclareReductionDecl>(DC))
+ return getEffectiveDeclContext(DR);
----------------
Use `isa<CapturedDecl>(DC) || isa<OMPDeclareReductionDecl>(DC)` then `cast<Decl>(DC)` here.
================
Comment at: lib/AST/ItaniumMangle.cpp:73
@@ -71,1 +72,3 @@
+ if (auto *DR = dyn_cast<OMPDeclareReductionDecl>(DC))
+ return getEffectiveDeclContext(DR);
----------------
Too much indentation here.
================
Comment at: lib/AST/MicrosoftMangle.cpp:63-64
@@ -61,2 +62,4 @@
return getEffectiveDeclContext(CD);
+ if (auto *DR = dyn_cast<OMPDeclareReductionDecl>(DC))
+ return getEffectiveDeclContext(DR);
----------------
Likewise.
================
Comment at: lib/CodeGen/CodeGenModule.h:1131
@@ +1130,3 @@
+ /// \brief Emit a code for declare reduction construct.
+ ///
+ void EmitOMPDeclareReduction(const OMPDeclareReductionDecl *D);
----------------
Remove blank comment line.
================
Comment at: lib/Parse/ParseOpenMP.cpp:86-143
@@ -82,1 +85,60 @@
+static DeclarationName parseOpenMPReductionId(Parser &P) {
+ DeclarationName Name;
+ const Token &Tok = P.getCurToken();
+ Sema &Actions = P.getActions();
+ switch (Tok.getKind()) {
+ case tok::plus: // '+'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("+"));
+ P.ConsumeToken();
+ break;
+ case tok::minus: // '-'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("-"));
+ P.ConsumeToken();
+ break;
+ case tok::star: // '*'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("*"));
+ P.ConsumeToken();
+ break;
+ case tok::amp: // '&'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("&"));
+ P.ConsumeToken();
+ break;
+ case tok::pipe: // '|'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("|"));
+ P.ConsumeToken();
+ break;
+ case tok::caret: // '^'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("^"));
+ P.ConsumeToken();
+ break;
+ case tok::ampamp: // '&&'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("&&"));
+ P.ConsumeToken();
+ break;
+ case tok::pipepipe: // '||'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("||"));
+ P.ConsumeToken();
+ break;
+ case tok::identifier: // identifier
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ Tok.getIdentifierInfo());
+ P.ConsumeToken();
+ break;
+ default:
+ P.Diag(Tok.getLocation(), diag::err_omp_expected_reduction_identifier);
+ P.SkipUntil(tok::colon, tok::r_paren, tok::annot_pragma_openmp_end,
+ Parser::StopBeforeMatch);
+ break;
+ }
+ return Name;
+}
+
----------------
Mapping the non-identifier operators to `IdentifierInfo`s is not the right approach. These should be a new kind of `DeclarationName` (you could reuse `CXXOperatorName`, but that's a bit of a hack, since that represents a name of the form `operator*`).
================
Comment at: lib/Parse/ParseOpenMP.cpp:94
@@ +93,3 @@
+ &Actions.Context.Idents.get("+"));
+ P.ConsumeToken();
+ break;
----------------
Maybe move the repeated `ConsumeToken` call out of the `switch`, and `return` from the `default` case?
================
Comment at: lib/Parse/ParseOpenMP.cpp:171-176
@@ +170,8 @@
+ // Consume ':'.
+ if (Tok.is(tok::colon)) {
+ ConsumeAnyToken();
+ } else {
+ Diag(Tok.getLocation(), diag::err_expected) << "':'";
+ IsCorrect = false;
+ }
+
----------------
Use `ExpectAndConsume(tok::colon)` for this.
================
Comment at: lib/Parse/ParseOpenMP.cpp:192
@@ +191,3 @@
+ bool IsCommaFound = false;
+ bool FunctionsCorrect = true;
+ // Parse list of types until ':' token.
----------------
You're not using this variable for anything.
================
Comment at: lib/Parse/ParseOpenMP.cpp:194
@@ +193,3 @@
+ // Parse list of types until ':' token.
+ while (Tok.isNot(tok::colon) && Tok.isNot(tok::annot_pragma_openmp_end)) {
+ ColonProtectionRAIIObject ColonRAII(*this);
----------------
You've already checked for colon and end-of-directive in the first iteration of this loop. You could instead move this check to the middle of the loop (before you check for a comma) and get rid of the `IsCommaFound` variable and some of the surrounding checks.
================
Comment at: lib/Parse/ParseOpenMP.cpp:200
@@ +199,3 @@
+ if (TR.isUsable()) {
+ QualType ReductionType = Sema::GetTypeFromParser(TR.get());
+ if (!ReductionType.isNull() &&
----------------
It's not OK to call this from the Parser. Pass the TypeResult back into Sema here.
================
Comment at: lib/Parse/ParseOpenMP.cpp:280-281
@@ +279,4 @@
+ // Parse <initializer> expression.
+ if (Tok.isAnyIdentifier() &&
+ Tok.getIdentifierInfo()->isStr("initializer")) {
+ ConsumeToken();
----------------
`isAnyIdentifier` isn't right here -- it returns true for `raw_identifiers` too (which you can't see here, and which would cause `getIdentifierInfo` to assert). `Tok.is(tok::identifier)` would be clearer.
================
Comment at: lib/Parse/ParseOpenMP.cpp:484
@@ +483,3 @@
+ }
+ SkipUntil(tok::annot_pragma_openmp_end);
+ break;
----------------
Maybe put this in an `else`?
================
Comment at: lib/Sema/SemaExpr.cpp:376-378
@@ +375,5 @@
+ auto *DRD = dyn_cast<OMPDeclareReductionDecl>(CurContext);
+ if (LangOpts.OpenMP && DRD && !CurContext->containsDecl(D) &&
+ isa<VarDecl>(D)) {
+ Diag(Loc, diag::err_omp_wrong_var_in_declare_reduction)
+ << getCurFunction()->HasOMPDeclareReductionCombiner;
----------------
You'll need a similar check elsewhere to prevent `this` from being used inside a function-scope `OMPDeclareReductionDecl`.
================
Comment at: lib/Sema/SemaLookup.cpp:1877-1878
@@ -1872,3 +1876,4 @@
case LookupLabel:
+ case LookupOMPReductionName:
// These lookups will never find a member in a C++ class (or base class).
return false;
----------------
You allow declaring a reduction at class scope, right? Should lookup for those not look into base classes?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6891
@@ -6888,1 +6890,3 @@
+bool Sema::isOpenMPDeclareReductionTypeAllowed(
+ SourceLocation TyLoc, QualType ReductionType,
----------------
Give this a name that makes it clearer that it emits diagnostics. Maybe `ActOnOpenMPDeclareReductionType`?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6919-6920
@@ +6918,4 @@
+ bool IsValid = true;
+ for (auto &&Data : RegisteredReductionTypes) {
+ if (Context.hasSameType(ReductionType, Data.first)) {
+ Diag(TyLoc, diag::err_omp_reduction_redeclared) << ReductionType;
----------------
An approach that is not quadratic-time in the number of reduction types would be better. (For instance, you could take the canonical types of the reductions and put them in a `DenseMap`.)
================
Comment at: lib/Sema/SemaOpenMP.cpp:6919-6926
@@ +6918,10 @@
+ bool IsValid = true;
+ for (auto &&Data : RegisteredReductionTypes) {
+ if (Context.hasSameType(ReductionType, Data.first)) {
+ Diag(TyLoc, diag::err_omp_reduction_redeclared) << ReductionType;
+ Diag(Data.second, diag::note_previous_declaration) << Data.second;
+ IsValid = false;
+ break;
+ }
+ }
+ return IsValid;
----------------
rsmith wrote:
> An approach that is not quadratic-time in the number of reduction types would be better. (For instance, you could take the canonical types of the reductions and put them in a `DenseMap`.)
It would be good to avoid the quadratic-time check here. Maybe delete this and perform the check in `...Start` below?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6943-6952
@@ +6942,12 @@
+ // rules.
+ if (S) {
+ LookupName(Lookup, S);
+ FilterLookupForScope(Lookup, DC, S, /*ConsiderLinkage=*/false,
+ /*AllowInlineNamespace=*/false);
+ } else
+ while (NextDeclInScope) {
+ auto *NextDRD = cast<OMPDeclareReductionDecl>(NextDeclInScope);
+ Lookup.addDecl(NextDRD);
+ NextDeclInScope = NextDRD->getNextDeclInScope();
+ }
+ OMPDeclareReductionDecl *PrevDeclInScope = nullptr;
----------------
Rather than populating a lookup result from the `NextDeclInScope` chains here, how about instead forming a `DenseMap<QualType, OMPDeclareReductionDecl*>` mapping the canonical types to their prior declarations (and computing `PrevDeclInScope` as you go)? Then just look up the type for each reduction in the loop below (and then add the new declaration to the map so you can check for redefinitions within a single list too).
================
Comment at: lib/Sema/SemaOpenMP.cpp:6947-6952
@@ +6946,8 @@
+ /*AllowInlineNamespace=*/false);
+ } else
+ while (NextDeclInScope) {
+ auto *NextDRD = cast<OMPDeclareReductionDecl>(NextDeclInScope);
+ Lookup.addDecl(NextDRD);
+ NextDeclInScope = NextDRD->getNextDeclInScope();
+ }
+ OMPDeclareReductionDecl *PrevDeclInScope = nullptr;
----------------
Add braces around this.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6963-6964
@@ +6962,4 @@
+ auto *PrevDecl = cast<OMPDeclareReductionDecl>(Filter.next());
+ if (!PrevDeclInScope && !PrevDecl->getNextDeclInScope())
+ PrevDeclInScope = PrevDecl;
+ if (PrevDecl->isInvalidDecl()) {
----------------
We should only build these chains for block-scope declarations. Otherwise, they're unnecessary and are going to be a problem for merging across modules.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6969-6971
@@ +6968,5 @@
+ }
+ if (Context.typesAreCompatible(TyData.first, PrevDecl->getType(),
+ /*CompareUnqualified=*/true)) {
+ Diag(S ? TyData.second : PrevDecl->getLocation(),
+ diag::err_omp_declare_reduction_redefinition)
----------------
Why do you use `typesAreCompatible` here and `hasSameType` above?
================
Comment at: lib/Sema/SemaOpenMP.cpp:7058
@@ +7057,3 @@
+ Context, DRD, D->getLocation(), &Context.Idents.get("omp_priv"),
+ Context.getLValueReferenceType(ReductionType));
+ if (S) {
----------------
It seems odd to create a reference type in C. Is this necessary? How's the behavior of this variable specified? (What should `decltype(omp_priv)` be?)
================
Comment at: lib/Sema/SemaOpenMP.cpp:7081-7085
@@ +7080,7 @@
+ DRD->setInvalidDecl();
+
+ if (Initializer)
+ DRD->setInitializer(Initializer);
+ else
+ DRD->setInvalidDecl();
+}
----------------
Duplicated code.
================
Comment at: lib/Sema/SemaOpenMP.cpp:7091-7094
@@ +7090,6 @@
+ DeclGroupPtrTy DeclReductions) {
+ // [OpenMP 4.0], 2.15 declare reduction Directive, Restrictions
+ // A reduction-identifier may not be re-declared in the current scope for the
+ // same type or for a type that is compatible according to the base language
+ // rules.
+ for (auto *D : DeclReductions.get()) {
----------------
This comment bears no relation to the adjacent code.
================
Comment at: lib/Sema/SemaTemplateInstantiate.cpp:2787-2788
@@ -2786,1 +2786,4 @@
+ // Local declare reduction constructs referenced prior to definition may
+ // require instantiation.
+ if (isa<OMPDeclareReductionDecl>(D))
----------------
Is this really possible? How?
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2458-2459
@@ +2457,4 @@
+ OMPDeclareReductionDecl *D) {
+ if (auto PrevInst = SemaRef.CurrentInstantiationScope->findInstantiationOf(D))
+ return PrevInst->get<Decl *>();
+ // Instantiate type and check if it is allowed.
----------------
How / why would you visit the same reduction twice?
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2474-2475
@@ +2473,4 @@
+ cast<OMPDeclareReductionDecl>(NextDeclInScope)->getNextDeclInScope();
+ if (NextDeclInScope)
+ NextDeclInScope = SemaRef.SubstDecl(NextDeclInScope, Owner, TemplateArgs);
+ }
----------------
This instantiates the declarations in the wrong order (we'll finish instantiating the last one in the scope before we finish instantiating the first one, and before we instantiate any intervening declarations that it might depend on). For instance, this will probably go badly:
template<typename T> void f() {
#pragma omp declare reduction (foo : int : omp_out += omp_in)
struct X { int k; };
#pragma omp declare reduction (foo : X : omp_out.k += omp_in.k)
}
... because we'll try to instantiate the second reduction before we instantiate `X`. Storing a previous pointer instead of a next pointer (and then looking up the previous decl in the current instantiation scope here) would fix this.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2489-2490
@@ +2488,4 @@
+ /*S=*/nullptr, NewDRD);
+ for (auto *Local : D->decls()) {
+ auto Lookup = NewDRD->lookup(cast<NamedDecl>(Local)->getDeclName());
+ if (!Lookup.empty()) {
----------------
The variables added for the initializer won't have been instantiated yet, so it looks like this won't add the right mappings to the current instantiation scope for them.
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2490
@@ +2489,3 @@
+ for (auto *Local : D->decls()) {
+ auto Lookup = NewDRD->lookup(cast<NamedDecl>(Local)->getDeclName());
+ if (!Lookup.empty()) {
----------------
It might be cleaner to just hardcode the two names that you expect to exist here (and likewise below).
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2380
@@ +2379,3 @@
+ D->setInitializer(Reader.ReadExpr(F));
+ D->setNextDeclInScope(ReadDeclAs<OMPDeclareReductionDecl>(Record, Idx));
+}
----------------
This forces deserialization of the whole chain whenever we deserialize any reduction. Maybe use a `LazyDeclPtr` here.
http://reviews.llvm.org/D11182
More information about the cfe-commits
mailing list