[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: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-22 19:41:52
Message-ID: de24e9f617d3020dbc1119ce7bb492e8 () localhost ! localdomain
[Download RAW message or body]
rsmith added a comment.
There are some problems and code duplication here caused by trying to shoehorn the multiple \
different reductions declared by a single pragma into one declaration. Instead, I suggest you \
do what we do for all other declarations that declare multiple entities: generate a separate \
declaration node for each entity. This loses a little source fidelity, since we no longer \
directly model which declarations came from the same textual declaration, but that's consistent \
with what we do elsewhere, and we should fix that with a single global approach (such as adding \
a `DeclGroupDecl`) rather than fixing it piecemeal.
================
Comment at: include/clang/AST/DeclBase.h:290
@@ -286,3 +289,3 @@
/// IdentifierNamespace - This specifies what IDNS_* namespace this lives in.
- unsigned IdentifierNamespace : 12;
+ unsigned IdentifierNamespace : 13;
----------------
This is not OK; it makes all `Decl`s 4 bytes larger (all the bits of this 32-bit bitfield were \
already in use). Can you take a bit out of `DeclKind`? (Please try to measure the performance \
impact of that change -- we probably `DeclKind` in a lot of places, and this will require us to \
mask off a bit rather than just performing a byte load.)
================
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 NamedDecl, public DeclContext {
+public:
----------------
Why is this a `DeclContext`?
================
Comment at: include/clang/AST/DeclOpenMP.h:125
@@ +124,3 @@
+ : NamedDecl(DK, DC, L, Name), DeclContext(DK), NumReductions(0) {
+ setModulePrivate();
+ }
----------------
Why should a reduction declared at namespace scope not be exported from its module?
================
Comment at: include/clang/AST/DeclOpenMP.h:159-160
@@ +158,4 @@
+ /// combiners and initializers \p CombinersInitializers.
+ void addReductions(ArrayRef<std::pair<QualType, SourceRange>> ReductionTypes,
+ ArrayRef<std::pair<Expr *, Expr *>> CombinersInitializers);
+
----------------
Given that these arrays are the same size, is there a reason to provide them separately? (Maybe \
pass in an `ArrayRef<ReductionData>`?) Also, can this be done directly by the `Create` \
function?
================
Comment at: include/clang/AST/DeclOpenMP.h:167-168
@@ +166,4 @@
+ bool reductions_empty() const { return NumReductions == 0; }
+ reductions_list reductions() { return getReductionData(); }
+ reductions_const_list reductions() const { return getReductionData(); }
+
----------------
We prefer AST nodes to be immutable after creation; is mutable access to the reductions \
necessary?
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7646-7649
@@ -7645,1 +7645,6 @@
"parent region for 'omp %select{cancellation point/cancel}0' construct cannot be ordered">;
+def err_omp_reduction_qualified_type : Error<"a type name cannot be qualified with 'const', \
'volatile' or 'restrict'">; +def err_omp_reduction_function_type : Error<"a type name cannot be \
a function type">; +def err_omp_reduction_reference_type : Error<"a type name cannot be a \
reference type">; +def err_omp_reduction_array_type : Error<"a type name cannot be an array \
type">; +def err_omp_reduction_redeclared : Error<"previous declaration with type %0 is \
found">;
----------------
These should say something more specific than "a type name". A type name can be any of these \
things; should this say something like "reduction type cannot be [...]"?
================
Comment at: lib/CodeGen/CGDecl.cpp:1811
@@ +1810,3 @@
+void CodeGenModule::EmitOMPDeclareReduction(const OMPDeclareReductionDecl *D) {
+ llvm_unreachable("Codegen for 'omp declare reduction' is not supported yet.");
+}
----------------
So, how is code generation for these supposed to work? Do they result in functions being \
emitted for the reduction and initialization steps? Are there name manglings defined for these?
================
Comment at: lib/Parse/ParseOpenMP.cpp:82
@@ +81,3 @@
+/// declare-reduction-directive:
+/// annot_pragma_openmp 'declare' 'reduction' '(' <reduction_id> ':'
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
----------------
Please specify what `<reduction_id>` is in this comment.
================
Comment at: lib/Parse/ParseOpenMP.cpp:82-85
@@ +81,6 @@
+/// declare-reduction-directive:
+/// annot_pragma_openmp 'declare' 'reduction' '(' <reduction_id> ':'
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
+/// ('omp_priv' '=' <expression>|<function_call>) ')']
+/// annot_pragma_openmp_end
+///
----------------
rsmith wrote:
> Please specify what `<reduction_id>` is in this comment.
Can you wrap this a bit better? (Maybe linebreaks and some indentation after the '(', each ':', \
and the ')')?
================
Comment at: lib/Parse/ParseOpenMP.cpp:84
@@ +83,3 @@
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
+/// ('omp_priv' '=' <expression>|<function_call>) ')']
+/// annot_pragma_openmp_end
----------------
This does not match your implementation. Is the initializer required to start 'omp_priv =' or \
not?
================
Comment at: lib/Parse/ParseOpenMP.cpp:99
@@ +98,3 @@
+
+ DeclarationName Name;
+ switch (Tok.getKind()) {
----------------
Please factor out a ParseOpenMPReductionId function.
================
Comment at: lib/Parse/ParseOpenMP.cpp:102-103
@@ +101,4 @@
+ case tok::plus: // '+'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("+"));
+ ConsumeAnyToken();
----------------
This is a really strange thing to do. What do these non-identifier names mean, and when can \
they be used? Would it make more sense to model these names as 'operator +' etc, rather than \
building a "+" identifier?
================
Comment at: lib/Parse/ParseOpenMP.cpp:252
@@ +251,3 @@
+ ExprResult CombinerResult =
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+ Actions.FinishOpenMPDeclareReductionCombiner(CombinerResult.get());
----------------
What is the lifetime of temporaries in the reduction expression? Should you \
`ActOnFinishFullExpr` here instead?
================
Comment at: lib/Parse/ParseOpenMP.cpp:285-286
@@ +284,4 @@
+ // Parse expression.
+ InitializerResult =
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+ IsCorrect = !Actions.FinishOpenMPDeclareReductionInitializer(
----------------
Again, is this a full-expression? What is the lifetime of temporaries created here?
================
Comment at: lib/Parse/ParseOpenMP.cpp:412-416
@@ -143,1 +411,7 @@
///
+/// declare-reduction-directive:
+/// annot_pragma_openmp 'declare' 'reduction' '(' <reduction_id> ':'
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
+/// ('omp_priv' '=' <expression>|<function_call>) ')']
+/// annot_pragma_openmp_end
+///
----------------
Seems excessive to list this grammar production three times. Maybe just
annot_pragma_openmp 'declare' 'reduction' [...]
here?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6585-6586
@@ +6584,4 @@
+ if (ReductionType->isFunctionType() ||
+ ReductionType->isFunctionNoProtoType() ||
+ ReductionType->isFunctionProtoType() ||
+ ReductionType->isFunctionPointerType() ||
----------------
This is redundant, these are both function types.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6587-6588
@@ +6586,4 @@
+ ReductionType->isFunctionProtoType() ||
+ ReductionType->isFunctionPointerType() ||
+ ReductionType->isMemberFunctionPointerType()) {
+ Diag(TyRange.getBegin(), diag::err_omp_reduction_function_type) << TyRange;
----------------
These cases produce an incorrect diagnostic ("cannot be a function type").
================
Comment at: lib/Sema/SemaOpenMP.cpp:6589-6597
@@ +6588,11 @@
+ ReductionType->isMemberFunctionPointerType()) {
+ Diag(TyRange.getBegin(), diag::err_omp_reduction_function_type) << TyRange;
+ return false;
+ }
+ if (ReductionType->isReferenceType()) {
+ Diag(TyRange.getBegin(), diag::err_omp_reduction_reference_type) << TyRange;
+ return false;
+ }
+ if (ReductionType->isArrayType()) {
+ Diag(TyRange.getBegin(), diag::err_omp_reduction_array_type) << TyRange;
+ return false;
----------------
Maybe fold these diagnostics into a single one with a parameter to %select which kind of bad \
type was provided?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6602
@@ +6601,3 @@
+ bool IsValid = true;
+ for (auto &&Data : RegisteredReductionTypes) {
+ if (Context.hasSameType(ReductionType, Data.first)) {
----------------
Use `auto &` rather than `auto &&` here; you're not binding to a temporary.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6603-6610
@@ +6602,10 @@
+ for (auto &&Data : RegisteredReductionTypes) {
+ if (Context.hasSameType(ReductionType, Data.first)) {
+ Diag(TyRange.getBegin(), diag::err_omp_reduction_redeclared)
+ << ReductionType << TyRange;
+ Diag(Data.second.getBegin(), diag::note_previous_declaration)
+ << Data.second;
+ IsValid = false;
+ break;
+ }
+ }
----------------
What happens if the same type is redeclared in a separate #pragma?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6644-6657
@@ +6643,16 @@
+
+ // Create 'T omp_in;' implicit param.
+ auto *OmpInParm = ImplicitParamDecl::Create(
+ Context, DRD, Loc, &Context.Idents.get("omp_in"), ReductionType);
+ // Create 'T &omp_out;' implicit param.
+ auto *OmpOutParm = ImplicitParamDecl::Create(
+ Context, DRD, Loc, &Context.Idents.get("omp_out"),
+ Context.getLValueReferenceType(ReductionType));
+ if (S) {
+ PushOnScopeChains(OmpInParm, S);
+ PushOnScopeChains(OmpOutParm, S);
+ } else {
+ DRD->addDecl(OmpInParm);
+ DRD->addDecl(OmpOutParm);
+ }
+}
----------------
Given that you only have one `DeclContext` for all the reductions in a reduction declaration, \
and each reduction has a different type, how does this work?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6669-6670
@@ +6668,4 @@
+ bool VisitDeclRefExpr(const DeclRefExpr *E) {
+ if (auto VD = dyn_cast<VarDecl>(E->getDecl())) {
+ if (!VD->hasLocalStorage()) {
+ SemaRef.Diag(E->getLocStart(),
----------------
Can you perform this check as you parse, rather than doing it after the fact?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6670-6678
@@ +6669,11 @@
+ if (auto VD = dyn_cast<VarDecl>(E->getDecl())) {
+ if (!VD->hasLocalStorage()) {
+ SemaRef.Diag(E->getLocStart(),
+ Combiner ? diag::err_omp_wrong_var_for_combiner
+ : diag::err_omp_wrong_var_for_initializer)
+ << E->getSourceRange();
+ SemaRef.Diag(VD->getLocation(), diag::note_defined_here)
+ << VD << VD->getSourceRange();
+ return true;
+ }
+ }
----------------
The check you perform here doesn't match the wording of your diagnostics. Suppose I write this:
#pragma omp declare reduction (foo : int : ({ int a = omp_in; a = a * 2; omp_out += a; }))
(or a similar example with a lambda-expression or block literal). Is that valid (as your code \
suggests) or ill-formed (as the text of your diagnostic suggests)?
What happens if the reduction expression indirectly references a global (through a function \
call, constructor, overloaded operator, ...)?
================
Comment at: lib/Sema/SemaOpenMP.cpp:6780-6783
@@ +6779,6 @@
+ for (size_t i1 = i + 1, e1 = e + 1; i1 < e1; ++i1) {
+ if (Context.typesAreCompatible(ReductionTypes[i].first,
+ ReductionTypes[i1].first,
+ /*CompareUnqualified=*/true)) {
+ Diag(ReductionTypes[i1].second.getBegin(),
+ diag::err_omp_declare_reduction_redefinition)
----------------
You already checked this in `isOpenMPDeclareReductionTypeAllowed`.
================
Comment at: lib/Sema/SemaOpenMP.cpp:6795-6797
@@ +6794,5 @@
+ S = getScopeForContext(DC);
+ if (S) {
+ LookupResult Lookup(*this, DRD->getDeclName(), DRD->getLocation(),
+ LookupOMPReductionName);
+ Lookup.suppressDiagnostics();
----------------
This is wrong for template instantiation. It's not possible to perform a scope-based lookup \
like this when instantiating a template; the scope information is already gone.
If you want this to work, you're going to need to store enough information to be able to \
reconstruct the set of reductions declared with the same name in the same block scope after the \
fact. (Perhaps you could add a pointer to the previous reduction with the same name in the same \
block scope to the Decl?)
================
Comment at: lib/Sema/SemaTemplateInstantiateDecl.cpp:2487-2489
@@ +2486,5 @@
+ /*S=*/nullptr, D->getLocation(), DRD, SubstReductionType);
+ for (auto *Local : D->noload_decls()) {
+ auto Lookup =
+ NewDRD->noload_lookup(cast<NamedDecl>(Local)->getDeclName());
+ if (!Lookup.empty()) {
----------------
Don't use `noload_*`. They're only for the `ASTReader`'s internal use.
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2369-2370
@@ +2368,4 @@
+void ASTDeclReader::VisitOMPDeclareReductionDecl(OMPDeclareReductionDecl *D) {
+ VisitDecl(D);
+ D->setDeclName(Reader.ReadDeclarationName(F, Record, Idx));
+ unsigned NumReductions = D->reductions_size();
----------------
Please call `VisitNamedDecl` instead of reimplementing it.
http://reviews.llvm.org/D11182
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/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