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

Repository
rL LLVM

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
290 ↗(On Diff #29662)

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
99 ↗(On Diff #29662)

Why is this a DeclContext?

125 ↗(On Diff #29662)

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

159–160 ↗(On Diff #29662)

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?

167–168 ↗(On Diff #29662)

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

include/clang/Basic/DiagnosticSemaKinds.td
7646–7649 ↗(On Diff #29662)

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
1811 ↗(On Diff #29662)

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
82 ↗(On Diff #29662)

Please specify what <reduction_id> is in this comment.

82–85 ↗(On Diff #29662)

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

84 ↗(On Diff #29662)

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

99 ↗(On Diff #29662)

Please factor out a ParseOpenMPReductionId function.

102–103 ↗(On Diff #29662)

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?

252 ↗(On Diff #29662)

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

285–286 ↗(On Diff #29662)

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

412–416 ↗(On Diff #29662)

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

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

here?

lib/Sema/SemaOpenMP.cpp
6585–6586 ↗(On Diff #29662)

This is redundant, these are both function types.

6587–6588 ↗(On Diff #29662)

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

6589–6597 ↗(On Diff #29662)

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

6602 ↗(On Diff #29662)

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

6603–6610 ↗(On Diff #29662)

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

6644–6657 ↗(On Diff #29662)

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?

6669–6670 ↗(On Diff #29662)

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

6670–6678 ↗(On Diff #29662)

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

6780–6783 ↗(On Diff #29662)

You already checked this in isOpenMPDeclareReductionTypeAllowed.

6795–6797 ↗(On Diff #29662)

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
2487–2489 ↗(On Diff #29662)

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

lib/Serialization/ASTReaderDecl.cpp
2369–2370 ↗(On Diff #29662)

Please call VisitNamedDecl instead of reimplementing it.

ABataev added inline comments.Jul 28 2015, 8:22 PM
include/clang/AST/DeclBase.h
290 ↗(On Diff #29662)

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

include/clang/AST/DeclOpenMP.h
99 ↗(On Diff #29662)

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.

125 ↗(On Diff #29662)

Removed.

include/clang/Basic/DiagnosticSemaKinds.td
7646–7649 ↗(On Diff #29662)

Ok, fixed

lib/CodeGen/CGDecl.cpp
1811 ↗(On Diff #29662)

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
82 ↗(On Diff #29662)

Added description from the standard

82–85 ↗(On Diff #29662)

Tried to improve it.

84 ↗(On Diff #29662)

No, not required. Fixed.

99 ↗(On Diff #29662)

Ok, done

102–103 ↗(On Diff #29662)

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

252 ↗(On Diff #29662)

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

285–286 ↗(On Diff #29662)

Fixed, thanks

412–416 ↗(On Diff #29662)

Ok, done

lib/Sema/SemaOpenMP.cpp
6589–6597 ↗(On Diff #29662)

Ok, done

6602 ↗(On Diff #29662)

Fixed

6603–6610 ↗(On Diff #29662)

It must be diagnosed. And it will be diagnosed later

6644–6657 ↗(On Diff #29662)

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.

6669–6670 ↗(On Diff #29662)

Ok, done

6670–6678 ↗(On Diff #29662)
  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.
6795–6797 ↗(On Diff #29662)

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

lib/Sema/SemaTemplateInstantiateDecl.cpp
2487–2489 ↗(On Diff #29662)

Ok, fixed

lib/Serialization/ASTReaderDecl.cpp
2369–2370 ↗(On Diff #29662)

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
3011 ↗(On Diff #30891)

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
95 ↗(On Diff #32792)

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.

99 ↗(On Diff #32792)

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.

99 ↗(On Diff #32792)

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

103–105 ↗(On Diff #32792)

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

106 ↗(On Diff #32792)

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.

109 ↗(On Diff #32792)

Store this as an OMPDeclareReductionDecl*.

130 ↗(On Diff #32792)

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

137 ↗(On Diff #32792)

Likewise.

144–146 ↗(On Diff #32792)

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
70–73 ↗(On Diff #32792)

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

73 ↗(On Diff #32792)

Too much indentation here.

lib/AST/MicrosoftMangle.cpp
63–64 ↗(On Diff #32792)

Likewise.

lib/CodeGen/CodeGenModule.h
1131 ↗(On Diff #32792)

Remove blank comment line.

lib/Parse/ParseOpenMP.cpp
86–143 ↗(On Diff #32792)

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 ↗(On Diff #32792)

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

171–176 ↗(On Diff #32792)

Use ExpectAndConsume(tok::colon) for this.

192 ↗(On Diff #32792)

You're not using this variable for anything.

194 ↗(On Diff #32792)

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 ↗(On Diff #32792)

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

280–281 ↗(On Diff #32792)

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.

484 ↗(On Diff #32792)

Maybe put this in an else?

lib/Sema/SemaExpr.cpp
376–378 ↗(On Diff #32792)

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

lib/Sema/SemaLookup.cpp
1877–1878 ↗(On Diff #32792)

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

lib/Sema/SemaOpenMP.cpp
6891 ↗(On Diff #32792)

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

6919–6920 ↗(On Diff #32792)

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

6919–6926 ↗(On Diff #32792)

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

6943–6952 ↗(On Diff #32792)

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

6947–6952 ↗(On Diff #32792)

Add braces around this.

6963–6964 ↗(On Diff #32792)

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.

6969–6971 ↗(On Diff #32792)

Why do you use typesAreCompatible here and hasSameType above?

7058 ↗(On Diff #32792)

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

7081–7085 ↗(On Diff #32792)

Duplicated code.

7091–7094 ↗(On Diff #32792)

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
2458–2459 ↗(On Diff #32792)

How / why would you visit the same reduction twice?

2474–2475 ↗(On Diff #32792)

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.

2489–2490 ↗(On Diff #32792)

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.

2490 ↗(On Diff #32792)

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

lib/Serialization/ASTReaderDecl.cpp
2380 ↗(On Diff #32792)

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
95 ↗(On Diff #32792)

Fixed, thanks

99 ↗(On Diff #32792)

Fixed

106 ↗(On Diff #32792)

Fixed

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

Threadprivates can, but declare reduction can't. Fixed

lib/Parse/ParseOpenMP.cpp
86–143 ↗(On Diff #32792)

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 ↗(On Diff #32792)

Thanks, forgot about this function.

484 ↗(On Diff #32792)

Reworked this a little bit.

lib/Sema/SemaExpr.cpp
376–378 ↗(On Diff #32792)

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
1877–1878 ↗(On Diff #32792)

Oops, good catch, thanks.

lib/Sema/SemaOpenMP.cpp
6891 ↗(On Diff #32792)

Agree, fixed this

6919–6926 ↗(On Diff #32792)

Reworked all this stuff

6943–6952 ↗(On Diff #32792)

Did something like this

7058 ↗(On Diff #32792)

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
2458–2459 ↗(On Diff #32792)

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

2474–2475 ↗(On Diff #32792)

Reworked

lib/Serialization/ASTReaderDecl.cpp
2380 ↗(On Diff #32792)

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 ↗(On Diff #37321)

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 ↗(On Diff #37321)

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 ↗(On Diff #37321)

Fixed, thanks.

lib/AST/Decl.cpp
1463 ↗(On Diff #37321)

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 ↗(On Diff #45149)

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

lib/Sema/SemaTemplateInstantiateDecl.cpp
2494 ↗(On Diff #45149)

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