Page MenuHomePhabricator

[OPENMP] Codegen for the 'omp for' with static schedule (non-chunked).

Authored by amusman on Oct 20 2014, 1:02 AM.



Please review the first patch with codegen of the 'omp for' directive. It implements the simplest case, which is used when no chunk_size is specified in the schedule(static) or no 'schedule' clause is specified - the iteration space is divided by the library into chunks that are approximately equal in size, and at most one chunk is distributed to each thread. In this case, we do not need an outer loop in each thread - each thread requests once which iterations range it should handle (using __kmpc_for_static_init runtime call) and then runs the inner loop on this range.

Diff Detail


Event Timeline

amusman updated this revision to Diff 15135.Oct 20 2014, 1:02 AM
amusman retitled this revision from to [OPENMP] Codegen for the 'omp for' with static schedule (non-chunked)..
amusman updated this object.
amusman edited the test plan for this revision. (Show Details)
amusman added a subscriber: Unknown Object (MLST).
rjmccall added inline comments.Dec 5 2014, 11:42 AM
276 ↗(On Diff #15135)

End with a comma, please.

Also, some of these enumerators (e.g. LastIterationOffset) clearly correspond to actual child expressions, and some of them (e.g. NonworksharingArraysOffset) do not. Please:

  • give the ArraysOffset enumerators a more distinctive name and
  • put comments on both of them to indicate their special purpose.

Also, there should really be a comment somewhere in the class — probably right above this enum — describing the overall layout of the sub-expressions. That comment should include the fact that there are all of these fixed children, but different numbers for different kinds, plus the three arrays of length CollapsedNum.

649 ↗(On Diff #15135)

This is now officially Too Many Arguments. Make a struct for passing them around, please.

154 ↗(On Diff #15135)

Is this a change in the invariants of OpenMPLocThreadIDMap? That should be documented.

537 ↗(On Diff #15135)

This comment should be more explicit: these enumerators are taken from the enum sched_type in kmp.h.

560 ↗(On Diff #15135)

All of these cases should return immediately instead of modifying a local.

577 ↗(On Diff #15135)

The switch should be exhaustive, and you should put an llvm_unreachable here.

600 ↗(On Diff #15135)

Please comment why this is reasonable.

272 ↗(On Diff #15135)

The method assumes that the loop has static schedule. You should probably change the method name to reflect that, and the comment should say "Given that a loop has static schedule..." rather than "If the loop has static schedule...". Same thing for Fini, below.

Alternatively, if it's not supposed to assume that the loop has static schedule, there's no reason to document the exact call that it makes here, and you should describe what the method does at a higher level.

296 ↗(On Diff #15135)

This comment should mention that nullptr has a special meaning.

305 ↗(On Diff #15135)

Same comment as above: this function assumes that the loop has static schedule.

312 ↗(On Diff #15135)

Please spell out Finish, even though it isn't spelled out in the runtime function name.

473 ↗(On Diff #15135)

Describe this in higher-level terms. "Skip the entire loop if we don't meet the precondition." You're not hardcoding anything about what the precondition looks like here.

528 ↗(On Diff #15135)

Don't crash. CodeGenFunction has a perfectly good ErrorUnsupported method you can use.

530 ↗(On Diff #15135)

Again, comment what you're actually doing. Something like "We're now done with the loop, so jump to the continuation block."

545 ↗(On Diff #15135)

Should this occur even if the precondition isn't met?

2395 ↗(On Diff #15135)

Yeah, see, this would be a great place to use that struct that includes all the expressions instead of spelling them all out again with terrible variable names.

2858 ↗(On Diff #15135)

Every time you build up a bunch of expressions that you intend to emit as their own independent statement, you should be calling ActOnFinishFullExpr afterwards. This is absolutely required if there's any potential of introducing C++ temporaries in these expressions.

amusman updated this revision to Diff 17236.Dec 12 2014, 8:41 AM

Hi John,

Thank you for review! I've fixed all your comments.

Best regards,

rjmccall edited edge metadata.Dec 12 2014, 11:25 AM

Looks great, thanks!

This revision was automatically updated to reflect the committed changes.