[OpenMP] Folding threadLimit and numThreads when single value in kernels

[josemonsalve2]

Summary

The device runtime contains several calls to __kmpc_get_hardware_num_threads_in_block
and __kmpc_get_hardware_num_blocks. If the thread_limit and the num_teams are constant,
these calls can be folded to the constant value.

In this patch we use the already introduced AAFoldRuntimeCall and the NumTeams and
NumThreads kernel attributes (to be introduced in a different patch) to fold these functions.
The code checks all the kernels, and if their attributes match, the functions are folded.

In the future we will explore specializing for multiple values of NumThreads and NumTeams.

Depends on D106390

[josemonsalve2]

The test is breaking the compiler. I need to change it to see if it passes or not.

[josemonsalve2]

Fixing syntax, removing unnecessary code. Changing a break
to a indicatePessimisticFixpoint()

[tianshilei1992]

Just a comment before reviewing the patch: please don't rebase the patch as D105787 has been reverted.

[josemonsalve2]

In D106033#2880245, @tianshilei1992 wrote:

Just a comment before reviewing the patch: please don't rebase the patch as D105787 has been reverted.

ACK

[JonChesterfield]

Renaming the functions and adding to OMPKinds.def et al looks separate to folding them, should land that first. Makes the functional diff easier to read and reduces the amount of churn if the functional change needs to be backed out.

openmp/libomptarget/deviceRTLs/common/src/omptarget.cu

98

git-clang-format HEAD^ may be useful for things like this

openmp/libomptarget/deviceRTLs/common/support.h

53

Noinline seems bad. Why? Also looks like the functions tagged noinline here are different to the ones that are renamed.

openmp/libomptarget/deviceRTLs/nvptx/src/target_impl.cu

97

rename the functions in amdgcn/src/target_impl.hip too please

[JonChesterfield]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

3614

I haven't read through this part, but if we can only fold it to a constant sometimes, we shouldn't mark the calls that survive noinline, as that'll be expensive for the cases that this pass misses.

[jdoerfert]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

3614

We are about to remove noinline from known runtime functions such that we can keep them around until we get to OpenMP-Opt as calls. This will have the effect we want without any drawbacks. Thus, adding noinline in the runitme will be totally fine.

[tianshilei1992]

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

208

Probably worth a separate patch to add them.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

3632

No need to have else here because the code above already returns.

3635

no need for else as well

3636

Directly indicate pessimistic state because we don't know clearly what the number is.

3646

This has to be updated as SimplifiedValue is not part of BooleanState. Refer to foldIsSPMDExecMode.

3652

Update accordingly

3705

This part needs to be changed. Refer to the trunk for more details. Basically it should be IRPosition::callsite_returned(*CI).

openmp/libomptarget/deviceRTLs/common/src/reduction.cu

200

Since this part is moved to another patch, they should go in this patch.

[josemonsalve2]

Modifying to adapt to most of the comments already in here. Will provide more detail soon. Tests are still failing.

[josemonsalve2]

Ups, missed one comment from Shilei

[josemonsalve2]

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

208

Do you mean just to add these 4 lines? Or should I add these lines to the front end patch D106298?

[jdoerfert]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

1766–1767

copy and paste.

3623

if (!CallerKernelInfoAA.ReachingKernelEntries.isValid())
  return indicatePessimisticFixpoint();

also below

3634

use early exit instead,
if (!...)

return ...

3643

no reaching kernels is fine, keep it at none (which is the default) and just return UNCHANGED.

3644

make this function take the string attribute such that we can have a single one and not two.

3702

leftover. (and you can also just pipe an IRPosition into errs())

[josemonsalve2]

• Created a single function receiving a string instead of one per attribute: foldKernelFnAttribute
• Removed pesimisticFixpoint if no kernel is found.
• Adding Check for the ReachingKernelEntries valid state
• Removed unnecesary comments

[josemonsalve2]

Format

[tianshilei1992]

LGTM with one nit. @jdoerfert WDYT?

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

208

It's better to add them to the patch where they are introduced.

[josemonsalve2]

My test is still failing, but it fails on an assertion on the changeToSPMD method:

opt: /home/josem/postdoc/llvm_stuff/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:2898: bool (anonymous namespace)::AAKernelInfoFunction::changeToSPMDMode(llvm::Attributor &): Assertion `ExecMode && "Kernel without exec mode?"' failed.

I'm trying to debug it, but if it is obvious for any of you let me know if it is something I am doing or I am just triggering another bug

[jdoerfert]

Some minor notes, assuming the tests passes properly after, LGTM

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

3615

3637

You should set CurrentAttrValue = NextAttrValue at the end of the loop.

3642

llvm/test/Transforms/OpenMP/get_hardware_num_threads_in_block_fold.ll

7

you need a variable KERNEL_NAME_exec_mode per kernel here. Look at other tests or what clang generates.

[josemonsalve2]

Fixing final comments from @jdoerfert

[JonChesterfield]

Can we add the pass that removes NOINLINE from these functions before landing this one?

[jhuber6]

In D106033#2905091, @JonChesterfield wrote:

Can we add the pass that removes NOINLINE from these functions before landing this one?

I landed D106482 which does this. One problem is that this was getting run on the .bc libraries and removing it prematurely. @tianshilei1992 was going to make a patch to stop running OpenMPOpt when we build the libraries for this.

[tianshilei1992]

In D106033#2905101, @jhuber6 wrote:

@tianshilei1992 was going to make a patch to stop running OpenMPOpt when we build the libraries for this.

It's already done. ;-)

[JonChesterfield]

Ah, good. I wasn't tagged on D106482 and missed the emails about it.

[josemonsalve2]

Rebasing to main

[josemonsalve2]

Sync to main

[josemonsalve2]

Resync again

[josemonsalve2]

Rebasing to main this time for real