[PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.
Alexey Bataev via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 14 00:53:48 PDT 2015
ABataev marked 40 inline comments as done.
ABataev added a comment.
Richard, thanks for the review! Tried to fix all your 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
----------------
rsmith wrote:
> 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.
Fixed, thanks
================
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:
> 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.
Fixed
================
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
----------------
rsmith wrote:
> 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.
Fixed
================
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
----------------
rsmith wrote:
> 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.
Threadprivates can, but declare reduction can't. Fixed
================
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;
+}
+
----------------
rsmith wrote:
> 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*`).
Reworked the whole function. I know about CXXOperatorName, but tried to simplify future lookup. Actually, I don't think that this is a hack, since OpenMP specifies that for non-identifier ops a form of 'operator op' is used.
================
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;
+ }
+
----------------
rsmith wrote:
> Use `ExpectAndConsume(tok::colon)` for this.
Thanks, forgot about this function.
================
Comment at: lib/Parse/ParseOpenMP.cpp:484
@@ +483,3 @@
+ }
+ SkipUntil(tok::annot_pragma_openmp_end);
+ break;
----------------
rsmith wrote:
> Maybe put this in an `else`?
Reworked this a little bit.
================
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;
----------------
rsmith wrote:
> You'll need a similar check elsewhere to prevent `this` from being used inside a function-scope `OMPDeclareReductionDecl`.
This cannot be used, because OMPDeclareReduction is very similar to static functions. Added a test, that checks that `this` cannot be used.
================
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;
----------------
rsmith wrote:
> You allow declaring a reduction at class scope, right? Should lookup for those not look into base classes?
Oops, good catch, thanks.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6891
@@ -6888,1 +6890,3 @@
+bool Sema::isOpenMPDeclareReductionTypeAllowed(
+ SourceLocation TyLoc, QualType ReductionType,
----------------
rsmith wrote:
> Give this a name that makes it clearer that it emits diagnostics. Maybe `ActOnOpenMPDeclareReductionType`?
Agree, fixed this
================
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:
> 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?
Reworked all this stuff
================
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;
----------------
rsmith wrote:
> 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).
Did something like this
================
Comment at: lib/Sema/SemaOpenMP.cpp:7058
@@ +7057,3 @@
+ Context, DRD, D->getLocation(), &Context.Idents.get("omp_priv"),
+ Context.getLValueReferenceType(ReductionType));
+ if (S) {
----------------
rsmith wrote:
> 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?)
This is a reference to private variable, which should be initialized by this pseudo-function. I could make it a pointer (and I would like to make it), but according to standard decltype(declrefexpr for omp_priv) must be T, not T*.
================
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.
----------------
rsmith wrote:
> How / why would you visit the same reduction twice?
This was because of reference to next declaration, not previous one. Fixed
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2474-2475
@@ +2473,4 @@
+ cast<OMPDeclareReductionDecl>(NextDeclInScope)->getNextDeclInScope();
+ if (NextDeclInScope)
+ NextDeclInScope = SemaRef.SubstDecl(NextDeclInScope, Owner, TemplateArgs);
+ }
----------------
rsmith wrote:
> 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.
Reworked
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2380
@@ +2379,3 @@
+ D->setInitializer(Reader.ReadExpr(F));
+ D->setNextDeclInScope(ReadDeclAs<OMPDeclareReductionDecl>(Record, Idx));
+}
----------------
rsmith wrote:
> This forces deserialization of the whole chain whenever we deserialize any reduction. Maybe use a `LazyDeclPtr` here.
Thanks, used LazyDeclPtr.
http://reviews.llvm.org/D11182
More information about the cfe-commits
mailing list