[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.
From: Alexey Bataev via cfe-commits <cfe-commits () lists ! llvm ! org>
Date: 2015-10-14 7:53:48
Message-ID: 15d06251f97780d2a140d0249b9f0576 () localhost ! localdomain
[Download RAW message or body]
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
_______________________________________________
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