This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Opt] Fix target region without parallel
ClosedPublic

Authored by doru1004 on Sep 1 2022, 9:15 AM.

Details

Summary

In the current setup the change to SPMD is abandoned if the target region does not contain any parallel regions. This actually does not stop the kernel from being considered SPMD by OpenMP Opt because this check is performed after the fixpoint is reached. This leads to an inconsistency between what the runtime thinks the execution mode is (generic) and what the code inside the kernel thinks the execution mode is (SPMD).

This patch fixes this problem by being more fine grained:

  1. eliminate instruction guarding if no parallel regions are present.
  2. since instructions aren't guarded anymore ensure that only the team main thread executes the target region.

Diff Detail

Event Timeline

doru1004 created this revision.Sep 1 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2022, 9:15 AM
doru1004 requested review of this revision.Sep 1 2022, 9:15 AM
Herald added a project: Restricted Project. · View Herald Transcript
doru1004 updated this revision to Diff 457306.Sep 1 2022, 9:30 AM
ronlieb added a subscriber: ronlieb.Sep 1 2022, 9:41 AM
doru1004 set the repository for this revision to rG LLVM Github Monorepo.
doru1004 added a project: Restricted Project.
doru1004 updated this revision to Diff 457357.Sep 1 2022, 12:39 PM
jdoerfert accepted this revision.Sep 1 2022, 2:38 PM

LG, minor nits only. I assume you can address them before committing, let me know if there is an issue. Thanks for fixing this properly.

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

Style: Given that we have now a conditional, let's move the two long code sequences in helper methods. It's hard to read like this as the "else" is easily over looked (by me).

Remove all the llvm:: above.

4151–4152

The above conditional should be deleted. This might cause more tests to be affected.

llvm/test/Transforms/OpenMP/get_hardware_num_threads_in_block_fold.ll
23

Put the comments before the function so they are not deleted when we auto-generate the check lines. Also, auto-generate the check lines ;)

This revision is now accepted and ready to land.Sep 1 2022, 2:38 PM
doru1004 updated this revision to Diff 457663.Sep 2 2022, 12:11 PM
doru1004 marked 3 inline comments as done.Sep 2 2022, 12:21 PM

Updated the patch to show all the comments have been addressed.

@jdoerfert if you could have a look again this time at the changes to the tests triggered by the removal of the conditional as follows:

For the spmdization test I had to add different prefixes to get the update script to not complain about conflicting code for the same CHECK and also prevent it from removing the generic state machine checks.
For the single threaded test I had to add a missing exec_mode global variable. The compiler was failing due to exec_mode variable missing (assert fail).

jdoerfert accepted this revision.Sep 2 2022, 1:00 PM

I think this looks good. Thanks for the updates.