Page MenuHomePhabricator

[OPENMP] Enable correct generation of runtime call when target directive is separated from teams directive by multiple curly brackets
ClosedPublic

Authored by carlo.bertolli on Mar 25 2016, 9:06 AM.

Details

Summary

This patch fixes a bug in code generation of the correct OpenMP runtime library call in presence of target and teams, when target is separated by teams with multiple curly brackets. The current implementation will not be able to see the teams directive inside target and issue a call to tgt_target instead of the correct one tgt_target_teams.

Diff Detail

Repository
rL LLVM

Event Timeline

carlo.bertolli retitled this revision from to [OPENMP] Enable correct generation of runtime call when target directive is separated from teams directive by multiple curly brackets.
carlo.bertolli updated this object.
carlo.bertolli set the repository for this revision to rL LLVM.
sfantao edited edge metadata.Mar 25 2016, 9:18 AM

Thanks for the fix!

lib/CodeGen/CGOpenMPRuntime.cpp
4265

I think that this can be simplified to:

while (auto *S = dyn_cast<CompoundStmt>(TargetBody))
  TargetBody = S->body_front();

return dyn_cast<CompoundStmt>(OMPTeamsDirective);

I know that this is currently used only for teams, but I think it would be nice to make this a templated function to look for other possible nests. I suspect this will very useful for other cases like 'target teams parallel distribute'.

carlo.bertolli added inline comments.Mar 25 2016, 9:57 AM
lib/CodeGen/CGOpenMPRuntime.cpp
4265

Thanks! I agree that having this as a template will help with combined directives. I will produce a new version of the patch with that support.

About your new loop: this would crash if S->body_front() is nullptr. This may happen if you have something like this:

#target
{
}

A solution may be

while (auto *S = dyn_cast_or_null<CompoundStmt>(TargetBody))
  TargetBody = S->body_front();

return dyn_cast_or_null<CompoundStmt>(OMPTeamsDirective);

Thoughts?

sfantao added inline comments.Mar 25 2016, 10:01 AM
lib/CodeGen/CGOpenMPRuntime.cpp
4265

You're totally right, dyn_cast_or_null is the right cast to use in the return statement.

carlo.bertolli edited edge metadata.

Simplify the search function and generalize by using a template.

ABataev added inline comments.Mar 29 2016, 8:19 PM
lib/CodeGen/CGOpenMPRuntime.cpp
4265

You don't need SK, because body_front() member function exists only in CompoundStmt.

carlo.bertolli marked an inline comment as done.Apr 11 2016, 10:53 AM

Alexey

Thanks for the review. I addressed your comment in a new version of the revision.
Samuel already reviewed the patch, as you can see in the message exchange. I am very happy to apply any design change you can think of.

  • Carlo

[OPENMP] Remove template for intervening statement and only allow CompoundStmt.

ABataev added inline comments.Apr 12 2016, 4:19 AM
lib/CodeGen/CGOpenMPRuntime.cpp
4260–4273

I don't like this function at all. We need to add a template function. Each instantiation will produce a new function with almost the same body. MI prepfer to see another solution, like:

static const Stmt *ignoreCompoundStmts(const Stmt *Body) {
  while (auto *CS = dyn_cast_or_null<CompoundStmt>(Body))
    Body = CS->body_front();
  return Body;
}
...
if (auto *TeamsDir = dyn_cast<OMPTeamsDirective>(ignoreCompoundStmts(CS.getCapturedStmt()))) {
...
carlo.bertolli marked an inline comment as done.Apr 12 2016, 10:26 AM

Great, thanks for the hint. I do much prefer your version of the function. I am updating the diff.

[OPENMP] Remove unnecessary templating of function.

ABataev accepted this revision.Apr 12 2016, 11:25 AM
ABataev edited edge metadata.

LG, except for the small comment

lib/CodeGen/CGOpenMPRuntime.cpp
4260

\brief tag is not required anymore, just a brief description is required.

This revision is now accepted and ready to land.Apr 12 2016, 11:25 AM
carlo.bertolli marked an inline comment as done.

Committed revision 267972.