This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
ClosedPublic

Authored by carlo.bertolli on Jun 21 2016, 12:09 PM.

Details

Reviewers
kkwli0
ABataev
Summary

This patch is an initial implementation for #distribute parallel for.
The main differences that affect other pragmas are:

  1. The implementation of 'distribute parallel for' requires blocking of the associated loop, where blocks are "distributed" to different teams and iterations within each block are scheduled to parallel threads within each team. To implement blocking, sema creates two additional worksharing directive fields that are used to pass the team assigned block lower and upper bounds through the outlined function resulting from 'parallel'. In this way, scheduling for 'for' to threads can use those bounds.
  2. As a consequence of blocking, the stride of 'distribute' is not 1 but it is equal to the blocking size. This is returned by the runtime and sema prepares a DistIncrExpr variable to hold that value.
  3. As a consequence of blocking, the global upper bound (EnsureUpperBound) expression of the 'for' is not the original loop upper bound (e.g. in for(i = 0 ; i < N; i++) this is 'N') but it is the team-assigned block upper bound. Sema creates a new expression holding the calculation of the actual upper bound for 'for' as UB = min(UB, PrevUB), where UB is the loop upper bound, and PrevUB is the team-assigned block upper bound.

Diff Detail

Repository
rL LLVM

Event Timeline

carlo.bertolli retitled this revision from to [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'.
carlo.bertolli updated this object.
carlo.bertolli added reviewers: ABataev, kkwli0.
carlo.bertolli set the repository for this revision to rL LLVM.
carlo.bertolli removed rL LLVM as the repository for this revision.

[OpenMP] Add regression test for distribute used as if-clause modifier and check that an error is generated

ABataev added inline comments.Jun 21 2016, 9:13 PM
include/clang/AST/StmtOpenMP.h
2884–2896

Don't like the idea of public fields with protected setters. If these fields are required, they must be the part of the base OMPLoopDirective and reuse the logic for other helper expressions.

Also, I believe, these fields are required for codegen. If so, it is better to add them in codegen patch

lib/CodeGen/CGStmtOpenMP.cpp
1868–1871

Could you just emit the captured statement as is, without dropping it?

ABataev requested changes to this revision.Jun 21 2016, 9:14 PM
ABataev edited edge metadata.
This revision now requires changes to proceed.Jun 21 2016, 9:14 PM
carlo.bertolli marked 2 inline comments as done.Jun 22 2016, 7:44 AM

Thanks very much for the quick review. I am about to update the patch reflecting your comments and to the latest trunk.

include/clang/AST/StmtOpenMP.h
2884–2896

I removed the field and will add new fields to OMPLoopDirective later on when I produce a code gen patch.

carlo.bertolli edited edge metadata.
carlo.bertolli set the repository for this revision to rL LLVM.
carlo.bertolli marked an inline comment as done.

[OpenMP] Update patch to trunk and to reflect comments. Remove fields from OMPDistributeParallelFor class and will add them to the related code gen patch.

ABataev requested changes to this revision.Jun 22 2016, 8:53 PM
ABataev edited edge metadata.
ABataev added inline comments.
lib/CodeGen/CGStmtOpenMP.cpp
1868–1871

No, it won't work. Use the following code:

OMPLexicalScope Scope(*this, S, /*AsInlined=*/true);
CGM.getOpenMPRuntime().emitInlinedDirective(
  *this, OMPD_distribute_parallel_for, [&S](CodeGenFunction &CGF, PrePostActionTy &) {
  OMPLoopScope PreInitScope(CGF, S);
  CGF.EmitStmt(cast<CapturedStmt>(S.getAssociatedStmt())->getCapturedStmt());
});
lib/Sema/SemaOpenMP.cpp
4984–4986

Not sure that this is a good solution. I'd prefer not to change the types of parameters. Maybe it is better to pass them as pointers and then cast to proper types?

This revision now requires changes to proceed.Jun 22 2016, 8:53 PM
carlo.bertolli edited edge metadata.
carlo.bertolli marked 2 inline comments as done.

[OpenMP] Address comments: emit statements in code gen as an inlined directive (no actual code gen, just a placeholder); change type of parallel outlined function parameters to pointer to enable casting in code gen to the right type, instead of inferring value from loop analysis.

Thanks for these further comments. I have just updated the diff applying the suggestions.

ABataev accepted this revision.Jun 23 2016, 7:46 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.Jun 23 2016, 7:46 PM

LG

lib/Sema/SemaOpenMP.cpp
1826–1827

Just a small comment - I believe, it is better to use SizeTy, rather than KmpUInt64Ty. SizeTy is platform dependent and on 32 bit targets may result in more effective code.

carlo.bertolli edited edge metadata.

[OpenMP] Change type of distribute lower and upper bound fields to portable size_t (Context.getSizeType()).

Thanks for the hint - I have updated the diff to use Context.getSizeType(). Please let me know if this is what you meant.

Committed revision 273705.

Resubmitted at revision 273884 after fixes.