This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Initial codegen for 'parallel for' directive.
ClosedPublic

Authored by ABataev on Mar 26 2015, 5:34 AM.

Details

Summary

Allows generation of combined 'parallel for' directive that represents 'parallel' region with internal implicit 'for' worksharing region.
A new combined region RAII is added to the runtime support module, intended to be used for all combined directives, like 'parallel sections', 'parallel for simd' etc.
This RAII region allows to split codegen for combined directives into several independent steps: at first it tries to generate the code for the outher implicit OpenMP region, then for internal. For example, for 'parallel for' directive on first iteration it generates an outlined function for parallel region, and then generates internal loop using information from the outer region. This region stores a kind of implicit directive for which it currently emits code and then advances to the next implicit directive. This allows to reuse the codegen emission method for combined directive for all implicitly included directives just like this:

if (<CurrentImplicitRegion> == OMPD_parallel)
  <Generate code for 'parallel' region>
else if (<CurrentImplicitRegion> == OMPD_for>
  <Generate code for 'for' region>
...

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 22709.Mar 26 2015, 5:34 AM
ABataev retitled this revision from to [OPENMP] Initial codegen for 'parallel for' directive..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
rjmccall edited edge metadata.Mar 27 2015, 10:34 AM

Looks good overall.

lib/CodeGen/CGOpenMPRuntime.cpp
226 ↗(On Diff #22709)

This is a weird API; its state is half internal (the current index) and half external (the actual array). It should really own both parts or neither. For example, I have no idea what the combined region info can actually do with "CurrentKindPos" without knowing the array.

ABataev added inline comments.Mar 29 2015, 10:27 PM
lib/CodeGen/CGOpenMPRuntime.cpp
226 ↗(On Diff #22709)

John, I agree. I also thought about it. I think I'd remove CurrentKindPos and will use nextCodeGenKindAtSpecified() everywhere.

ABataev updated this revision to Diff 22871.Mar 30 2015, 3:06 AM
ABataev edited edge metadata.

Update after review, reworked processing of the list of implicit regions in codegen for combined directives.

rjmccall added inline comments.Mar 30 2015, 10:06 PM
lib/CodeGen/CGOpenMPRuntime.h
51 ↗(On Diff #22871)

This isn't really a Kind, and the name makes it really confusing. It's not a "range" either in the usual C++ sense of a pair of iterators. It's really a generator, although that name is also misleading in the broader context of IRGen.

I think this is probably just not the right abstraction. Would it make more sense for the emission of the combining directives to take a lambda (probably as an llvm::FunctionRef) that fills in the outlined function body? "parallel" would just pass a lambda that just emits the captured statement, while "parallel for" would pass a lambda that builds a "for" around the captured statement. As it is, there is a lot of hard-to-follow recursion here.

ABataev added inline comments.Mar 30 2015, 10:51 PM
lib/CodeGen/CGOpenMPRuntime.h
51 ↗(On Diff #22871)

John, this solution would work for 'simple' combined constructs, like 'parallel for' or 'parallel sections'. But what with combined directives like 'target teams distribute parallel for simd'? Here we have two outlined functions (one for 'teams', one for 'parallel' regions) with several inlined regions ('target' in the outer scope, 'distribute' in first outlined function and 'for simd' in the second outlined region). It seems to me recursion is much better for such complex cases.

ABataev added inline comments.Mar 30 2015, 11:59 PM
lib/CodeGen/CGOpenMPRuntime.h
51 ↗(On Diff #22871)

Or another option - to pass a set of lambdas responsible for codegen of each particular region (i.e. passing of codegen methods instead of list of directives)

ABataev updated this revision to Diff 23567.Apr 9 2015, 11:38 PM

Updated patch in accordance with the latest changes in OpenMP API.

This revision was automatically updated to reflect the committed changes.