[PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.
Richard Smith
richard at metafoo.co.uk
Wed Jul 22 12:41:52 PDT 2015
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
More information about the cfe-commits
mailing list