Page MenuHomePhabricator

[OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class
ClosedPublic

Authored by fghanim on Feb 13 2020, 8:31 AM.

Details

Summary

This patch introduces a new helper class OMPBuilderCBHelpers,
which will contain all reusable C/C++ language specific function-
alities required by the OMPIRBuilder.

Initially, this helper class contains the body and finalization
codegen functionalities implemented using callbacks which were
moved here for reusability among the different directives
implemented in the OMPIRBuilder, along with RAIIs for preserving
state prior to emitting outlined and/or inlined OpenMP regions.

In the future this helper class will also contain all the different
call backs required by OpenMP clauses/variable privatization.

Diff Detail

Event Timeline

fghanim created this revision.Feb 13 2020, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2020, 8:31 AM

This is great, thanks a lot! I only have two comments where I am not sure I understand the code/change.

clang/lib/CodeGen/CGStmtOpenMP.cpp
3135–3136

These TODOs are now obsolete ;)

clang/lib/CodeGen/CodeGenFunction.h
354

Don't we have to set/reset the CGF.ReturnBlock ? If not, why do we need FinilizationBlock here?

clang/test/OpenMP/parallel_codegen.cpp
143 ↗(On Diff #244443)

Do you know why this changed? Is this variable used in the parallel region?

fghanim updated this revision to Diff 245014.Feb 17 2020, 10:49 AM
fghanim marked 2 inline comments as done.

Fixed bug where variables where still being allocated in original function entry block

Regarding the third comment (which was removed for some reason), This new update should fix that

clang/lib/CodeGen/CGStmtOpenMP.cpp
3135–3136

We need to split our patches to keep them small :p

clang/lib/CodeGen/CodeGenFunction.h
354

Yes, and we still do for outlined regions (check OutlinedRegionBodyRAII ctor above).

This is for inlined regions, and those shouldn't change the CGF.ReturnBlock. It should re-use the CGF.ReturnBlock for the enclosing outlined region.
The only reason I kept it, is because CGF.getJumpDestInCurrentScope() was updating a counter that seemed to me to relate to EHStack, and I didn't want to remove it without investigating first.

jdoerfert accepted this revision.Feb 17 2020, 4:05 PM

LGTM assuming the comment below is addressed.

clang/lib/CodeGen/CodeGenFunction.h
354

I see. Maybe remove the FinilizationBlock member though. Call getJumpDestInCurrentScope and add a comment (TODO) that explains this, e.g., says it has to be investigated further.

This revision is now accepted and ready to land.Feb 17 2020, 4:05 PM
fghanim updated this revision to Diff 245151.EditedFeb 18 2020, 6:50 AM

addressing review comments - Adding a comment to explain minor change.

fghanim marked 3 inline comments as done.Feb 18 2020, 6:51 AM
fghanim marked an inline comment as done.
fghanim updated this revision to Diff 245227.Feb 18 2020, 11:46 AM

Marking a call void

I am done updating this patch.
I still don't have commit access, I'd appreciate it if you'd commit this for me when you get a chance.

This revision was automatically updated to reflect the committed changes.