Page MenuHomePhabricator

[OpenMP] Codegen support for 'target parallel' on the host.
ClosedPublic

Authored by arpith-jacob on Jan 15 2017, 4:44 PM.

Details

Summary

This patch adds support for codegen of 'target parallel' on the host.
It is also the first combined directive that requires two or more
captured statements. Support for this functionality is included in
the patch.

A combined directive such as 'target parallel' has two captured
statements, one for the 'target' and the other for the 'parallel'
region. Two captured statements are required because each has
different implicit parameters (see SemaOpenMP.cpp). For example,
the 'parallel' has 'global_tid' and 'bound_tid' while the 'target'
does not. The patch adds support for handling multiple captured
statements based on the combined directive.

When codegen'ing the 'target parallel' directive, the 'target'
outlined function is created using the outer captured statement
and the 'parallel' outlined function is created using the inner
captured statement.

Diff Detail

Repository
rL LLVM

Event Timeline

arpith-jacob retitled this revision from to [OpenMP] Codegen support for 'target parallel' on the host..
arpith-jacob updated this object.
arpith-jacob added a subscriber: cfe-commits.
ABataev added inline comments.Jan 16 2017, 12:29 AM
include/clang/Sema/Sema.h
8328–8330 ↗(On Diff #84510)

I think it would be good to have a member function that returns the captured statement for the provided OpenMP region kind, like CapturedStmt *getCapturedStmt(OpenMPDirectiveKind Kind). In this case you don't need to add an additional parameter for the codegen functions

lib/Sema/SemaOpenMP.cpp
1914 ↗(On Diff #84510)

} // namespace

2001–2003 ↗(On Diff #84510)

No braces here

lib/Sema/TreeTransform.h
7243–7245 ↗(On Diff #84510)

No braces

Added a method 'getCapturedStmt' as part of OMPExecutableDirective.

arpith-jacob marked 4 inline comments as done.Jan 16 2017, 8:48 PM

Updated 'getOpenMPCaptureRegions' to return the OMPD_teams region kind for the teams directive.

ABataev added inline comments.Jan 17 2017, 6:01 AM
lib/CodeGen/CGOpenMPRuntime.h
543 ↗(On Diff #84629)

I don't think you need to pass a new CS parameter here, you can use InnermostKind param as an arg to getCapturedStmt() function

arpith-jacob added inline comments.Jan 17 2017, 8:20 AM
lib/CodeGen/CGOpenMPRuntime.h
543 ↗(On Diff #84629)

Hi Alexey, this is tricky.

emitParallelOrTeamsOutlinedFunction() is called from emitCommonOMPParallelDirective(...InnermostKind...). Here is how that function is called for 'parallel for'.

void CodeGenFunction::EmitOMPParallelForDirective(
    const OMPParallelForDirective &S) {
  ...
  emitCommonOMPParallelDirective(*this, S, /*InnermostKind=*/OMPD_for, CodeGen);
}

You'll notice that InnermostKind is OMPD_for. OMPD_for does not have a CapturedStmt, only the OMPD_parallel part. So we cannot use the InnermostKind variable.

Here is an alternative solution:

I will split emitParallelOrTeamsOutlinedFunction() to emitParallelOutlinedFunction() and emitTeamsOutlinedFunction(). I can then use getCapturedStmt(OMPD_parallel) and getCapturedStmt(OMPD_teams) respectively and I don't have to pass the CS parameter.

I also think this makes sense because apart from the host, teams and parallel codegen are very different and so they should be in different routines.

Let me know if you disagree.

ABataev added inline comments.Jan 17 2017, 9:17 AM
lib/CodeGen/CGOpenMPRuntime.h
543 ↗(On Diff #84629)

Arpith, I'm ok with your idea

The patch was updated to split 'emitParallelOrTeamsOutlinedFunction' into 'emitParallelOutlinedFunction' and 'emitTeamsOutlinedFunction' to enable the use of getCapturedStmt().

Also updated an assert statement for correctness and the switch in getCapturedStmt() to not depend on the input argument.

ABataev accepted this revision.Jan 18 2017, 4:40 AM

LG with nits

lib/CodeGen/CGOpenMPRuntime.cpp
848–860 ↗(On Diff #84689)

s/outlinedHelperName/OutlinedHelperName/g

lib/Sema/SemaOpenMP.cpp
1933–1937 ↗(On Diff #84689)

Could you move this code to CaptureRegionUnwinderRAII?

This revision is now accepted and ready to land.Jan 18 2017, 4:40 AM
arpith-jacob added inline comments.Jan 18 2017, 4:50 AM
lib/Sema/SemaOpenMP.cpp
1933–1937 ↗(On Diff #84689)

Hi Alexey, this code will be expanded in the next patch. Have a look at:
https://reviews.llvm.org/D28781
SemaOpenMP.cpp:1930.
This is the code to codegen preinits. I think that code doesn't belong to the RAII. That's why I haven't moved this code to CaptureRegionUnwinderRAII.

Let me know if you disagree.

This revision was automatically updated to reflect the committed changes.