[PATCH] D11182: [OPENMP 4.0] Initial support for 'omp declare reduction' construct.
Alexey Bataev
a.bataev at hotmail.com
Tue Jul 28 20:22:39 PDT 2015
ABataev added inline comments.
================
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;
----------------
rsmith wrote:
> 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.)
Done. Tried to measure comp time - did not find any significant changes.
================
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:
----------------
rsmith wrote:
> Why is this a `DeclContext`?
Actually declare reduction is very similar to functions. It implicitly declares at least 4 internal variables - omp_in, omp_out, omp_priv and omp_orig.
================
Comment at: include/clang/AST/DeclOpenMP.h:125
@@ +124,3 @@
+ : NamedDecl(DK, DC, L, Name), DeclContext(DK), NumReductions(0) {
+ setModulePrivate();
+ }
----------------
rsmith wrote:
> Why should a reduction declared at namespace scope not be exported from its module?
Removed.
================
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">;
----------------
rsmith wrote:
> 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 [...]"?
Ok, fixed
================
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.");
+}
----------------
rsmith wrote:
> 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?
Yes, there should be reduction/initialization functions emitted. No, there is no specific manglings for these functions, their names are not specified, so we could define our own rules.
================
Comment at: lib/Parse/ParseOpenMP.cpp:82
@@ +81,3 @@
+/// declare-reduction-directive:
+/// annot_pragma_openmp 'declare' 'reduction' '(' <reduction_id> ':'
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
----------------
rsmith wrote:
> 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 ')')?
Added description from the standard
================
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
+///
----------------
ABataev wrote:
> rsmith wrote:
> > 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 ')')?
> Added description from the standard
Tried to improve it.
================
Comment at: lib/Parse/ParseOpenMP.cpp:84
@@ +83,3 @@
+/// <type> {',' <type>} ':' <expression> ')' ['initializer' '('
+/// ('omp_priv' '=' <expression>|<function_call>) ')']
+/// annot_pragma_openmp_end
----------------
rsmith wrote:
> This does not match your implementation. Is the initializer required to start 'omp_priv =' or not?
No, not required. Fixed.
================
Comment at: lib/Parse/ParseOpenMP.cpp:99
@@ +98,3 @@
+
+ DeclarationName Name;
+ switch (Tok.getKind()) {
----------------
rsmith wrote:
> Please factor out a ParseOpenMPReductionId function.
Ok, done
================
Comment at: lib/Parse/ParseOpenMP.cpp:102-103
@@ +101,4 @@
+ case tok::plus: // '+'
+ Name = Actions.getASTContext().DeclarationNames.getIdentifier(
+ &Actions.Context.Idents.get("+"));
+ ConsumeAnyToken();
----------------
rsmith wrote:
> 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?
I thought about it, but the problem here is that it will make the code more complex (especially lookup procedure) without any benefits.
================
Comment at: lib/Parse/ParseOpenMP.cpp:252
@@ +251,3 @@
+ ExprResult CombinerResult =
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+ Actions.FinishOpenMPDeclareReductionCombiner(CombinerResult.get());
----------------
rsmith wrote:
> What is the lifetime of temporaries in the reduction expression? Should you `ActOnFinishFullExpr` here instead?
Yes, you're right. Will replace it by ActOnFinishFullExpr.
================
Comment at: lib/Parse/ParseOpenMP.cpp:285-286
@@ +284,4 @@
+ // Parse expression.
+ InitializerResult =
+ Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+ IsCorrect = !Actions.FinishOpenMPDeclareReductionInitializer(
----------------
rsmith wrote:
> Again, is this a full-expression? What is the lifetime of temporaries created here?
Fixed, thanks
================
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
+///
----------------
rsmith wrote:
> Seems excessive to list this grammar production three times. Maybe just
>
> annot_pragma_openmp 'declare' 'reduction' [...]
>
> here?
Ok, done
================
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;
----------------
rsmith wrote:
> Maybe fold these diagnostics into a single one with a parameter to %select which kind of bad type was provided?
Ok, done
================
Comment at: lib/Sema/SemaOpenMP.cpp:6602
@@ +6601,3 @@
+ bool IsValid = true;
+ for (auto &&Data : RegisteredReductionTypes) {
+ if (Context.hasSameType(ReductionType, Data.first)) {
----------------
rsmith wrote:
> Use `auto &` rather than `auto &&` here; you're not binding to a temporary.
Fixed
================
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;
+ }
+ }
----------------
rsmith wrote:
> What happens if the same type is redeclared in a separate #pragma?
It must be diagnosed. And it will be diagnosed later
================
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);
+ }
+}
----------------
rsmith wrote:
> 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?
Each time we start a reduction for new type I pop pseudo parameters from the scope chains, so the lookup proceadure each time sees only variables with the specific type. But I'll split the construct.
================
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(),
----------------
rsmith wrote:
> Can you perform this check as you parse, rather than doing it after the fact?
Ok, done
================
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;
+ }
+ }
----------------
rsmith wrote:
> 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, ...)?
1. Yes, this is valid. Because anyway, you're still using only 2 external variables. 'a' is internal.
2. It is unspecified behavior, we have nothing to do with this.
================
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();
----------------
rsmith wrote:
> 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?)
I know about this problem and thought to solve it later. Thanks for advice, I will try it.
================
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()) {
----------------
rsmith wrote:
> Don't use `noload_*`. They're only for the `ASTReader`'s internal use.
Ok, fixed
================
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();
----------------
rsmith wrote:
> Please call `VisitNamedDecl` instead of reimplementing it.
Ok, fixed
http://reviews.llvm.org/D11182
More information about the cfe-commits
mailing list