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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
