Page MenuHomePhabricator

[AbstractAttributor] Fold function calls to `__kmpc_is_spmd_exec_mode` if possible
ClosedPublic

Authored by tianshilei1992 on Jul 11 2021, 6:03 PM.

Details

Summary

In the device runtime there are many function calls to __kmpc_is_spmd_exec_mode
to query the execution mode of current kernels. In many cases, user programs
only contain target region executing in one mode. As a consequence, those runtime
function calls will only return one value. If we can get rid of these function
calls during compliation, it can potentially improve performance.

In this patch, we use AAKernelInfo to analyze kernel execution. Basically, for
each kernel (device) function F, we collect all kernel entries K that can
reach F. A new AA, AAFoldRuntimeCall, is created for each call site. In each
iteration, it will check all reaching kernel entries, and update the folded value
accordingly.

In the future we will support more function.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Tests will come soon

tianshilei1992 added inline comments.Jul 11 2021, 6:40 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3560

This might not be always true. What if later there is a kernel entry in another mode?

add test case and update some code. note: test case is not right currently. keep debugging.

tianshilei1992 retitled this revision from [AbstractAttributor] Fold function calls to `__kmpc_is_spmd_exec_mode` if possible to [WIP][AbstractAttributor] Fold function calls to `__kmpc_is_spmd_exec_mode` if possible.Jul 11 2021, 7:32 PM

We need to properly tack when SPMDCompatibility is known.

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

I don't think we need this anymore, do we?

3426

Do we need/want a KernelInfoState here? Wouldn't a BooleanState suffice to do the tacking of valid/invalid and fixpoint. Plus the Optional<Constant *> SimplifiedValue;.

3475
3560

I don't think that can happen

tianshilei1992 added inline comments.Jul 11 2021, 7:37 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3560

Function call in is_mixed_helper in the test case is folded by mistake.

tianshilei1992 added inline comments.Jul 11 2021, 7:39 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3426

Nope because we access CallerKernelInfoAA.ReachingKernelEntries directly in foldIsSPMDExecMode

fixed issue that assumed information is not being used

tianshilei1992 marked 4 inline comments as done.Jul 12 2021, 4:59 PM
tianshilei1992 added inline comments.Jul 12 2021, 5:10 PM
llvm/test/Transforms/OpenMP/is_spmd_exec_mode_fold.ll
95

The function call here can only be folded if we use assumed information as SPMDization doesn't change the state to known.

removed value simplification call back registration as it can affect SPMDization

not use assumed information

refined getAsStr and changed to BooleanState

rebase and reuse assumed information

Minor nits, basically good to go.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3527–3530

Same below please.

3558

Technically we could even try to stay with nullptr but not pessimistic fixppoint here.

3580

Remove the empty conditionals above.

llvm/test/Transforms/OpenMP/is_spmd_exec_mode_fold.ll
130

Can we have a second is_generic helper like this:

define internal void @is_non_spmd_helper2() {
;
  %isSPMD = call i8 @__kmpc_is_spmd_exec_mode()
  %c = icmp eq i8 %isSPMD, 0
  br i1 %c, label %t, label %f
t:
  call void @foo()
  ret void
f:
  call void bar()
  ret void
}
tianshilei1992 marked 4 inline comments as done.

rebase and fix comments

reorg some code

jdoerfert accepted this revision.Jul 13 2021, 5:15 PM

Change the title of the commit, clang format. LGTM

This revision is now accepted and ready to land.Jul 13 2021, 5:15 PM

rebase, fix tests, clang-formats

tianshilei1992 retitled this revision from [WIP][AbstractAttributor] Fold function calls to `__kmpc_is_spmd_exec_mode` if possible to [AbstractAttributor] Fold function calls to `__kmpc_is_spmd_exec_mode` if possible.Jul 13 2021, 5:57 PM
This revision was landed with ongoing or failed builds.Jul 13 2021, 7:28 PM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Jul 15 2021, 8:24 AM

fixed an issue that insertion of ReachingKernelEntries invalidates it

tianshilei1992 requested review of this revision.Jul 15 2021, 10:19 AM
jdoerfert accepted this revision.Jul 15 2021, 10:23 AM

LG, one nit.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3566–3570

below we might get to all generic but here we can give up

This revision is now accepted and ready to land.Jul 15 2021, 10:23 AM
tianshilei1992 marked an inline comment as done.Jul 15 2021, 10:32 AM

fixed comments

jdoerfert added inline comments.Jul 15 2021, 11:39 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
3250

I don't think we need this anymore, do we?

fixed a series of critical issues

This revision was landed with ongoing or failed builds.Jul 15 2021, 3:23 PM
This revision was automatically updated to reflect the committed changes.

Thank you for the revert, it has fixed many failures in the buildbot openmp-offload-cuda-project (https://lab.llvm.org/staging/#/builders/155/builds/1885) with only 5 remaining (4 by PR49940, PR50738 and PR50739; and 1 caused by either D101976 or D101977)

Strangely, openmp-offload-cuda-runtime only followed much later when the next clean build was pending (https://lab.llvm.org/staging/#/builders/154/builds/1509). There might be a dependency problem in the LLVM_ENABLE_RUNTIMES build.

Thank you for the revert, it has fixed many failures in the buildbot openmp-offload-cuda-project (https://lab.llvm.org/staging/#/builders/155/builds/1885) with only 5 remaining (4 by PR49940, PR50738 and PR50739; and 1 caused by either D101976 or D101977)

Strangely, openmp-offload-cuda-runtime only followed much later when the next clean build was pending (https://lab.llvm.org/staging/#/builders/154/builds/1509). There might be a dependency problem in the LLVM_ENABLE_RUNTIMES build.

but I committed it again after all known issues have been fixed. Does it look good now?

ye-luo added a subscriber: ye-luo.Jul 17 2021, 8:05 AM

@tianshilei1992
There are still issues remaining after you reapply the patch.
One of them seems being fixed by 97c8f60bbaf0986320fdfd03b11328b91c730a96
But more remaining
https://bugs.llvm.org/show_bug.cgi?id=51124

@tianshilei1992
There are still issues remaining after you reapply the patch.
One of them seems being fixed by 97c8f60bbaf0986320fdfd03b11328b91c730a96
But more remaining
https://bugs.llvm.org/show_bug.cgi?id=51124

Thanks for the report. Would you mind giving https://reviews.llvm.org/D106209 a shot to see if it can fix the issue?