This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Codegen for distribute directive
ClosedPublic

Authored by carlo.bertolli on Feb 11 2016, 1:43 PM.

Details

Summary

This patch provide basic implementation of codegen for teams directive, excluding all clauses except dist_schedule. It also fixes parts of AST reader/writer to enable correct pre-compiled header handling.

Diff Detail

Repository
rL LLVM

Event Timeline

carlo.bertolli retitled this revision from to [OPENMP] Codegen for distribute directive.
carlo.bertolli updated this object.
carlo.bertolli set the repository for this revision to rL LLVM.
ABataev added inline comments.Feb 11 2016, 9:33 PM
lib/CodeGen/CGOpenMPRuntime.h
669–692

I don't like the idea of exposing this in the header file. getRuntimeSchedule() can be turned into static functions in .cpp file

717–725

What's the difference between isStaticNonchunked() and isDistStaticNonchunked()? For me they must be the same.

759

I don't think this function is required at all. You can just add an optional argument to emitForStaticInit() function.

761

It is a bad idea to expose runtime-specific type in interface functions. Use OpenMPDistScheduleClauseKind instead and translate it inside.

lib/CodeGen/CGStmtOpenMP.cpp
1540–1541

Bad formatting

2185–2216

Wait few days for this code. I will commit improved code for schedule clause codegen, that simplifies all similar stuff

lib/CodeGen/CodeGenFunction.h
2405

This must be public along with other codegen function for other directives

Reflect changes in D17148 and show full context.

carlo.bertolli added inline comments.Feb 16 2016, 6:29 AM
lib/CodeGen/CGOpenMPRuntime.h
524

I added this because OpenMPScheduleClauseKind and OpenMPDistScheduleClauseKind are two different unrelated types. Do you suggest something like the following?

void emitForStaticInit(CodeGenFunction &CGF, SourceLocation Loc, OpenMPScheduleClauseKind ScheduleKind, OpenMPDistScheduleClauseKind ScheduleKind, unsigned IVSize, bool IVSigned, bool Ordered, Address IL, Address LB, Address UB, Address ST, llvm::Value *Chunk);

and for dist_schedule called like:

OpenMPDistScheduleClauseKind DistScheduleKind = ..;

RT.emitForStaticInit(*this, S.getLocStart(), OMPC_SCHEDULE_unknown, DistScheduleKind, IVSize, IVSigned, Ordered, IL, LB, UB, ST, Chunk);

Similarly, for schedule clause:

OpenMPScheduleClauseKind ScheduleKind = ..;

RT.emitForStaticInit(*this, S.getLocStart(), ScheduleKind, OMPC_DIST_SCHEDULE_unknown, IVSize, IVSigned, Ordered, IL, LB, UB, ST, Chunk);

lib/CodeGen/CGOpenMPRuntime.h
524

Another solution is to have a generic schedule type as a union of the schedule and dist_schedule enum types.

union OMPGenericSchedule {
  OpenMPScheduleClauseKind forSchedule;
  OpenMPDistScheduleClauseKind distributeSchedule;
};

Then we can decide which case we are in as following:

void CGOpenMPRuntime::emitForStaticInit(CodeGenFunction &CGF,
                                        SourceLocation Loc,
                                        OMPGenericSchedule SchedKind,
                                        bool isForSchedule,
                                        unsigned IVSize, bool IVSigned,
                                        bool Ordered, Address IL, Address LB,
                                        Address UB, Address ST,
                                        llvm::Value *Chunk) {

  OpenMPSchedType Schedule = (isForSchedule) ?
      getRuntimeSchedule(SchedKind.forSchedule, Chunk != nullptr, Ordered) :
      getRuntimeSchedule(SchedKind.distributeSchedule, Chunk != nullptr);
carlo.bertolli marked an inline comment as done.Feb 16 2016, 12:58 PM

I have addressed some of the comments and restructured the code as requested.
Let me know about further comments.
Thanks for making this a better patch!

lib/CodeGen/CGOpenMPRuntime.h
469–492

Fixed - now everything related to the openmp library is encapsulated back into the CGOpenMPRuntime.cpp file.

491–499

No difference: deleted the latter.

lib/CodeGen/CGStmtOpenMP.cpp
1566–1567

If this is about missing comments for booleans - done.
Otherwise, please be specific about the bad formatting I did.

2186–2303

I will wait until a new patch related to this is submitted - if it has not already.

lib/CodeGen/CodeGenFunction.h
2406

Made EmitOMPDistributeLoop public. What else needs to be public?

I have addressed some of the comments and restructured the code as requested.
Let me know about further comments.
Thanks for making this a better patch!

Address comments and rebase over new version of D17148.

ABataev edited edge metadata.Feb 16 2016, 9:29 PM

Carlo, thanks for the patch! Please update the code to trunk HEAD

lib/CodeGen/CGOpenMPRuntime.h
477

Add comment for the function

491–503

I don't like union also. I think we can avoid using this

523

Add separate function, but outline the body for these 2 functions into a static function

lib/CodeGen/CGStmtOpenMP.cpp
1566–1567

There was a problem with indentation, seems to me it is fixed

2174–2303

I reworked it already, check the code in trunk

2865–2866

Comment for 'true' arg?

carlo.bertolli marked 4 inline comments as done.Feb 17 2016, 9:20 AM
carlo.bertolli added inline comments.
lib/CodeGen/CGStmtOpenMP.cpp
2865–2866

I was missing a merge against my own connected patch..sorry!

carlo.bertolli edited edge metadata.

Updated diff reflecting comments.
Note that having the emission of the runtime kmpc_for_static_init function in a static function (instead of a method of CGOpenMPRuntime) required moving to public some fields in CGOpenMPRuntime used when calling the runtime function.
This is painful and comments are welcome on how to overcome this, if needed.

Update to new version of D17148, integrating changes from the trunk.

ABataev added inline comments.Feb 18 2016, 5:38 AM
lib/CodeGen/CGOpenMPRuntime.cpp
2071–2073

I think you can pass OpenMPSchedType here instead of OpenMPScheduleClauseKind and OpenMPDistScheduleClauseKind .

lib/CodeGen/CGOpenMPRuntime.h
52–73

No, do not make it public. It must be private forever. Moreover, It would be good if could hide it at all in .cpp file. I will look at it.

367–380

Keep them private, please

lib/CodeGen/CGStmtOpenMP.cpp
1872–1873

Revert it back, no actual changes

lib/Serialization/ASTReaderStmt.cpp
2303

Reformat it, please, indentation is wrong

lib/Serialization/ASTWriterStmt.cpp
2091

Indentation

carlo.bertolli marked 4 inline comments as done.

Partially address comments to replace schedule type with schedule num in interface of static function generating call to kmpc_for_static_init. Still waiting for updates on private members of CGOpenMPRuntime.cpp

lib/CodeGen/CGOpenMPRuntime.h
52–73

Waiting for you.

367–380

Waiting for you - If the basic emission routine for kmpc_for_static_init needs to be a static function in CGOpenMPRuntime.cpp, then this must be accessible somehow.

Update to trunk and implement suggestions from latest review: do not make public methods used in static function calling kmpc_for_static_init, but call those functions in the caller that can access them.

ABataev added inline comments.Feb 19 2016, 8:39 PM
lib/CodeGen/CGOpenMPRuntime.cpp
2126

Wrong indentation

2127

Just 'emitUpdateLocation(CGF, Loc)' please

2141

Also, 'emitUpdateLocation(CGF, Loc)'

carlo.bertolli marked 5 inline comments as done.Feb 22 2016, 8:36 AM

Updating patch following comments.

Address latest comments.

ABataev accepted this revision.Feb 25 2016, 7:40 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.Feb 25 2016, 7:40 PM

Committed revision 262741.