Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.