Page MenuHomePhabricator

[OpenMP][Opt] Delete read-only parallel regions
ClosedPublic

Authored by jdoerfert on Nov 7 2019, 9:47 AM.

Details

Summary

If a parallel region is known to be read-only, e.g., after we removed
all dead write accesses in their, we can remove the parallel region as
well.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 7 2019, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2019, 9:47 AM
Herald added a subscriber: jfb. · View Herald Transcript
jdoerfert updated this revision to Diff 228363.Nov 7 2019, 9:24 PM

Run the old-PM and keep it updated

Aside from the inline comment about functions that don't return, this looks good to me. Simple code, heavily tested. Thanks

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
139

I'm not totally confident that onlyReadsMemory is sufficient here (concerned about I/O, functions that do not return).

LICM hoists based on this so it's safe for I/O. Likewise malloc/free are fine.

Please could you point me to the reason this is safe for functions that don't return?

jdoerfert marked an inline comment as done.Dec 17 2019, 7:53 AM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
139

Please could you point me to the reason this is safe for functions that don't return?

It is not. I'll add the proper checks, we have the attribute (willreturn) so it is not a problem to look for it.

I/O is not read only. Malloc/free are neither.

It seems to be an openmp restriction that parallel regions can't exit/abort/return etc, so the current path looks fine. Perhaps an assertion on the attribute?

JonChesterfield accepted this revision.Dec 17 2019, 5:36 PM
This revision is now accepted and ready to land.Dec 17 2019, 5:36 PM
This revision was automatically updated to reflect the committed changes.