This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP 4.0] Initial support for 'omp declare reduction' construct.
ClosedPublic

Authored by ABataev on Jul 14 2015, 4:09 AM.

Details

Summary

Add parsing, sema analysis and serialization/deserialization for 'declare reduction' construct.
User-defined reductions are defined as

#pragma omp declare reduction( reduction-identifier : typename-list : combiner ) [initializer ( initializer-expr )]

These custom reductions may be used in 'reduction' clauses of OpenMP constructs. The combiner specifies how partial results can be combined into a single value. The
combiner can use the special variable identifiers omp_in and omp_out that are of the type of the variables being reduced with this reduction-identifier. Each of them will
denote one of the values to be combined before executing the combiner. It is assumed that the special omp_out identifier will refer to the storage that holds the resulting
combined value after executing the combiner.
As the initializer-expr value of a user-defined reduction is not known a priori the initializer-clause can be used to specify one. Then the contents of the initializer-clause
will be used as the initializer for private copies of reduction list items where the omp_priv identifier will refer to the storage to be initialized. The special identifier
omp_orig can also appear in the initializer-clause and it will refer to the storage of the original variable to be reduced.

Diff Detail

Event Timeline

ABataev updated this revision to Diff 29662.Jul 14 2015, 4:09 AM
ABataev retitled this revision from to [OPENMP 4.0] Initial support for 'omp declare reduction' construct..
ABataev updated this object.
ABataev added a reviewer: rsmith.
rsmith edited edge metadata.Jul 22 2015, 12:41 PM

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.

include/clang/AST/DeclBase.h
297

This is not OK; it makes all Decls 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.)

include/clang/AST/DeclOpenMP.h
100

Why is this a DeclContext?

126

Why should a reduction declared at namespace scope not be exported from its module?

160–161

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?

168–169

We prefer AST nodes to be immutable after creation; is mutable access to the reductions necessary?

include/clang/Basic/DiagnosticSemaKinds.td
7726–7729

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 [...]"?

lib/CodeGen/CGDecl.cpp
1812

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?

lib/Parse/ParseOpenMP.cpp
89

Please specify what <reduction_id> is in this comment.

89–92

Can you wrap this a bit better? (Maybe linebreaks and some indentation after the '(', each ':', and the ')')?

91

This does not match your implementation. Is the initializer required to start 'omp_priv =' or not?

106

Please factor out a ParseOpenMPReductionId function.

109–110

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?

259

What is the lifetime of temporaries in the reduction expression? Should you ActOnFinishFullExpr here instead?

292–293

Again, is this a full-expression? What is the lifetime of temporaries created here?

386–390

Seems excessive to list this grammar production three times. Maybe just

annot_pragma_openmp 'declare' 'reduction' [...]

here?

lib/Sema/SemaOpenMP.cpp
7419–7420

This is redundant, these are both function types.

7421–7422

These cases produce an incorrect diagnostic ("cannot be a function type").

7423–7431

Maybe fold these diagnostics into a single one with a parameter to %select which kind of bad type was provided?

7436

Use auto & rather than auto && here; you're not binding to a temporary.

7437–7444

What happens if the same type is redeclared in a separate #pragma?

7478–7491

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?

7503–7504

Can you perform this check as you parse, rather than doing it after the fact?

7504–7512

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, ...)?

7614–7617

You already checked this in isOpenMPDeclareReductionTypeAllowed.

7629–7631

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?)

lib/Sema/SemaTemplateInstantiateDecl.cpp
2507–2509

Don't use noload_*. They're only for the ASTReader's internal use.

lib/Serialization/ASTReaderDecl.cpp
2354–2355

Please call VisitNamedDecl instead of reimplementing it.

ABataev added inline comments.Jul 28 2015, 8:22 PM
include/clang/AST/DeclBase.h
297

Done. Tried to measure comp time - did not find any significant changes.

include/clang/AST/DeclOpenMP.h
100

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.

126

Removed.

include/clang/Basic/DiagnosticSemaKinds.td
7726–7729

Ok, fixed

lib/CodeGen/CGDecl.cpp
1812

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.

lib/Parse/ParseOpenMP.cpp
89

Added description from the standard

89–92

Tried to improve it.

91

No, not required. Fixed.

106

Ok, done

109–110

I thought about it, but the problem here is that it will make the code more complex (especially lookup procedure) without any benefits.

259

Yes, you're right. Will replace it by ActOnFinishFullExpr.

292–293

Fixed, thanks

386–390

Ok, done

lib/Sema/SemaOpenMP.cpp
7423–7431

Ok, done

7436

Fixed

7437–7444

It must be diagnosed. And it will be diagnosed later

7478–7491

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.

7503–7504

Ok, done

7504–7512
  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.
7629–7631

I know about this problem and thought to solve it later. Thanks for advice, I will try it.

lib/Sema/SemaTemplateInstantiateDecl.cpp
2507–2509

Ok, fixed

lib/Serialization/ASTReaderDecl.cpp
2354–2355

Ok, fixed

ABataev edited edge metadata.Jul 29 2015, 5:18 AM
ABataev updated this revision to Diff 30891.Jul 29 2015, 5:18 AM

Update after review

lib/Parse/ParseDeclCXX.cpp
3069–3070

While testing this patch with the latest trunk for my work on declare target, I kept getting a build error in ParseDeclCXX.cpp as there seems to be no CurAS in scope, this may be because of recent changes. But this line:

if (Tok.is(tok::annot_pragma_openmp))
  return ParseOpenMPDeclarativeDirective(CurAS);

Seems to work better as

if (Tok.is(tok::annot_pragma_openmp))
  return ParseOpenMPDeclarativeDirective(AS);

I'll update patch, Michael

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

20.08.2015 18:40, Michael Wong пишет:

fraggamuffin added a comment.

Comment at: lib/Parse/ParseDeclCXX.cpp:3011
@@ -3010,3 +3010,3 @@

if (Tok.is(tok::annot_pragma_openmp)) {
  • ParseOpenMPDeclarativeDirective(); continue; ----------------

While testing this patch with the latest trunk for my work on declare target, I kept getting a build error in ParseDeclCXX.cpp as there seems to be no CurAS in scope, this may be because of recent changes. But this line:

if (Tok.is(tok::annot_pragma_openmp))
  return ParseOpenMPDeclarativeDirective(CurAS);

Seems to work better as

if (Tok.is(tok::annot_pragma_openmp))
  return ParseOpenMPDeclarativeDirective(AS);

http://reviews.llvm.org/D11182

ABataev updated this revision to Diff 32792.Aug 20 2015, 9:22 PM

Updated to latest version.

Got it. Test results look good with this patch.

Expected Passes    : 7011
Expected Failures  : 21
Unsupported Tests  : 89
Unexpected Failures: 47
rsmith added inline comments.Oct 5 2015, 5:52 PM
include/clang/AST/DeclOpenMP.h
96

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.

100

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.

100

Why is this a DeclaratorDecl rather than merely a NamedDecl? It doesn't have a declarator, or even a type.

104–106

Store these as Expr*s. We use Stmt* arrays in Expr nodes to support StmtIterator, which is not a concern here.

107

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.

110

Store this as an OMPDeclareReductionDecl*.

131

This is redundant with the next function. If you keep both, this should return const Expr*.

138

Likewise.

145–147

Likewise.

lib/AST/ASTContext.cpp
8321–8322 ↗(On Diff #32792)

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.

lib/AST/ItaniumMangle.cpp
69–73

Use isa<CapturedDecl>(DC) || isa<OMPDeclareReductionDecl>(DC) then cast<Decl>(DC) here.

72

Too much indentation here.

lib/AST/MicrosoftMangle.cpp
63–64

Likewise.

lib/CodeGen/CodeGenModule.h
1115

Remove blank comment line.

lib/Parse/ParseOpenMP.cpp
86–143

Mapping the non-identifier operators to IdentifierInfos 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*).

94

Maybe move the repeated ConsumeToken call out of the switch, and return from the default case?

171–176

Use ExpectAndConsume(tok::colon) for this.

192

You're not using this variable for anything.

194

You've already checked for colon and end-of-directive in the first iteration of this loop. You could instead move this check to the middle of the loop (before you check for a comma) and get rid of the IsCommaFound variable and some of the surrounding checks.

200

It's not OK to call this from the Parser. Pass the TypeResult back into Sema here.

280–281

isAnyIdentifier isn't right here -- it returns true for raw_identifiers too (which you can't see here, and which would cause getIdentifierInfo to assert). Tok.is(tok::identifier) would be clearer.

449

Maybe put this in an else?

lib/Sema/SemaExpr.cpp
377–379

You'll need a similar check elsewhere to prevent this from being used inside a function-scope OMPDeclareReductionDecl.

lib/Sema/SemaLookup.cpp
1935–1936

You allow declaring a reduction at class scope, right? Should lookup for those not look into base classes?

lib/Sema/SemaOpenMP.cpp
7404

Give this a name that makes it clearer that it emits diagnostics. Maybe ActOnOpenMPDeclareReductionType?

7432–7433

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.)

7432–7439

It would be good to avoid the quadratic-time check here. Maybe delete this and perform the check in ...Start below?

7456–7465

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).

7460–7465

Add braces around this.

7476–7477

We should only build these chains for block-scope declarations. Otherwise, they're unnecessary and are going to be a problem for merging across modules.

7482–7484

Why do you use typesAreCompatible here and hasSameType above?

7571

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?)

7594–7598

Duplicated code.

7604–7607

This comment bears no relation to the adjacent code.

lib/Sema/SemaTemplateInstantiate.cpp
2787–2788 ↗(On Diff #32792)

Is this really possible? How?

lib/Sema/SemaTemplateInstantiateDecl.cpp
2477–2478

How / why would you visit the same reduction twice?

2493–2494

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.

2508–2509

The variables added for the initializer won't have been instantiated yet, so it looks like this won't add the right mappings to the current instantiation scope for them.

2509

It might be cleaner to just hardcode the two names that you expect to exist here (and likewise below).

lib/Serialization/ASTReaderDecl.cpp
2358

This forces deserialization of the whole chain whenever we deserialize any reduction. Maybe use a LazyDeclPtr here.

ABataev marked 40 inline comments as done.Oct 14 2015, 12:53 AM

Richard, thanks for the review! Tried to fix all your comments.

include/clang/AST/DeclOpenMP.h
96

Fixed, thanks

100

Fixed

107

Fixed

lib/AST/ASTContext.cpp
8321–8322 ↗(On Diff #32792)

Threadprivates can, but declare reduction can't. Fixed

lib/Parse/ParseOpenMP.cpp
86–143

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.

171–176

Thanks, forgot about this function.

449

Reworked this a little bit.

lib/Sema/SemaExpr.cpp
377–379

This cannot be used, because OMPDeclareReduction is very similar to static functions. Added a test, that checks that this cannot be used.

lib/Sema/SemaLookup.cpp
1935–1936

Oops, good catch, thanks.

lib/Sema/SemaOpenMP.cpp
7404

Agree, fixed this

7432–7439

Reworked all this stuff

7456–7465

Did something like this

7571

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*.

lib/Sema/SemaTemplateInstantiateDecl.cpp
2477–2478

This was because of reference to next declaration, not previous one. Fixed

2493–2494

Reworked

lib/Serialization/ASTReaderDecl.cpp
2358

Thanks, used LazyDeclPtr.

ABataev updated this revision to Diff 37321.Oct 14 2015, 12:54 AM
ABataev marked 16 inline comments as done.

Update after review

hfinkel added inline comments.Oct 27 2015, 11:11 AM
include/clang/Basic/DiagnosticParseKinds.td
995

We're not incredibly consistent here, but I think this reads better if we say:

'&&', or '||'

instead of:

'&&' and '||'

(adding the Oxford comma and 'and' -> 'or').

lib/AST/Decl.cpp
1463

are -> is

ABataev marked 2 inline comments as done.Nov 16 2015, 2:43 AM
ABataev added inline comments.
include/clang/Basic/DiagnosticParseKinds.td
995

Fixed, thanks.

lib/AST/Decl.cpp
1463

Fixed

ABataev updated this revision to Diff 40262.Nov 16 2015, 2:44 AM
ABataev marked 2 inline comments as done.

Update after review

Richard, could you take a look one more time, please?

Richard, could you take a look one more time, please?

ABataev updated this revision to Diff 44848.Jan 14 2016, 3:04 AM

Updated to latest version + some fixes and improvements

ABataev updated this revision to Diff 45149.Jan 17 2016, 10:03 PM

Simplified handling of private copies in C/C++. Now all outpup parameters of UDR must be passed by pointer, not by reference (better compatibility with C)

rsmith accepted this revision.Mar 2 2016, 6:31 PM
rsmith edited edge metadata.

Sorry for the delay, LGTM

lib/Sema/SemaExpr.cpp
377

(LangOpts.OpenMP != 0u) -> LangOpts.OpenMP
(DRD != nullptr) -> DRD

lib/Sema/SemaTemplateInstantiateDecl.cpp
2489

(PrevDeclInScope != nullptr) -> PrevDeclInScope

This revision is now accepted and ready to land.Mar 2 2016, 6:31 PM
This revision was automatically updated to reflect the committed changes.