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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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 |
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); |
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. | |
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!
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? |
lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
2865–2866 | I was missing a merge against my own connected patch..sorry! |
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.
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 |
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.
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.