This is an archive of the discontinued LLVM Phabricator instance.

[openmp][mlir] Lower parallel if to new fork_call_if function.
ClosedPublic

Authored by DavidTruby on Nov 22 2022, 6:26 AM.

Details

Summary

This patch adds a new runtime function fork_call_if and uses that
to lower parallel if statements when going through OpenMPIRBuilder.

This fixes an issue where the OpenMPIRBuilder passes all arguments to
fork_call as a struct but this struct is not filled corretly in the
non-if branch by handling the fork inside the runtime.

Diff Detail

Event Timeline

DavidTruby created this revision.Nov 22 2022, 6:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby requested review of this revision.Nov 22 2022, 6:26 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript

This is intended to fix https://github.com/llvm/llvm-project/issues/58233 .
Does this look like the right sort of approach to push the fork handling into the runtime?

jdoerfert accepted this revision.Nov 22 2022, 8:34 AM

Nice. Thanks for fixing this the right way.

This revision is now accepted and ready to land.Nov 22 2022, 8:34 AM

Looks fine to me.

There are clang-format issues.

changed files:
    llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
    openmp/runtime/src/kmp.h
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
965

Does this need an update since there is no ThenBB?

1028

Nit: This comment needs an update.

mlir/test/Target/LLVMIR/openmp-llvm.mlir
197

Was there any particular reason to remove this test? Can you update this test instead?

openmp/runtime/src/kmp_csupport.cpp
344

Would we ever want a variable length parameter option like the kmpc_fork_call?

347

Nit: Use braces here to match the else.

351

Would this launch a separate thread?

DavidTruby added inline comments.Nov 28 2022, 6:50 AM
openmp/runtime/src/kmp_csupport.cpp
344

As long as this is used only by the OpenMPIRBuilder this parameter will always be a struct. Possibly there would be some use to changing it to a VA_ARGS function like the others for consistency, but I tend to prefer to avoid them whenever possible :)

351

No, this will just call the outlined function directly from the main thread (or I guess from whichever thread called __kmpc_fork_call_if)

DavidTruby added inline comments.Nov 28 2022, 6:52 AM
mlir/test/Target/LLVMIR/openmp-llvm.mlir
197

This test was testing whether the tid.addr and zero.addr values were calculated in the correct basic block when the blocks were nested. Since these are now calculated inside __kmpc_fork_call_if when needed this test doesn't actually test anything any more.

Fix failing unit test

LG. Please check whether a unit test is required for kmpc_fork_call_if before submitting.

DavidTruby updated this revision to Diff 479316.Dec 1 2022, 8:51 AM

Make compatible with C and add unit test.

Adding the unit test also exposed an additional bug where no arguments are
passed if the region doesn't capture any variables. Instead we create a void
pointer to pass to fork_call_if in that case, and then check if that argument
exists before passing it on.

DavidTruby updated this revision to Diff 480480.Dec 6 2022, 7:08 AM

Fix failing MLIR lit tests

@jdoerfert I've had to change this quite a bit to cover some corner cases that came up, are you still happy with it?

This revision was landed with ongoing or failed builds.Dec 9 2022, 6:25 AM
This revision was automatically updated to reflect the committed changes.
mstorsjo added inline comments.
openmp/runtime/src/kmp.h
3904

This new function needs to be added to the dllexports file to make it available and usable on Windows.

DavidTruby added inline comments.Dec 12 2022, 6:57 AM
openmp/runtime/src/kmp.h
3904

Ok, I can add that; do you have any clue how the numbers for functions are chosen in that file? I don't really understand how dll exporting works on Windows

mstorsjo added inline comments.Dec 12 2022, 12:31 PM
openmp/runtime/src/kmp.h
3904

In general cases, it would be enough to just add the name (or mark the symbol with a dllexport attribute in the source code), but here in OpenMP, the exports also are given ordinal numbers (which need to be unique). I think the logic so far is to just pick the next free number.

D131830 is adding 292 and 293 (if it ends up reapplied in its current form), so I guess we could go with 294 to avoid conflicts - unless anyone of the actual OpenMP maintainers have a better suggestion?

mstorsjo added inline comments.
openmp/runtime/src/kmp.h
3904

CC @tianshilei1992 who has sorted out such issues before too.

I'd like to have this issue solved soonish in one way or another - this has had my nightly builds broken for many days already.

mstorsjo added inline comments.Dec 13 2022, 3:18 PM
openmp/runtime/src/kmp.h
3904

What do the maintainers, who I hope are reading this, prefer here - that I push a patch that just adds this function to dllexports with a free ordinal number picked for the function - or revert the patch that was applied last week so that we get it fixed with proper process?

openmp/runtime/src/kmp.h
3904

@DavidTruby was/is not working today. If you can add a fix that would be great.

mstorsjo added inline comments.Dec 14 2022, 4:42 AM
openmp/runtime/src/kmp.h
3904

I pushed a fix for this in 15151315f76b0840423bd3bd62c5f9fc647d31c6 now.

openmp/runtime/src/kmp.h
3904

Thanks @mstorsjo.

tianshilei1992 added inline comments.Sep 5 2023, 7:28 PM
openmp/runtime/src/kmp_csupport.cpp
356

This doesn't work on ARM64. The last argument is not pushed to stack but it is accessed in the callee.

    0x10021a84c <+188>: 0x910053e0   add    x0, sp, #0x14
    0x10021a850 <+192>: 0x910043e1   add    x1, sp, #0x10
    0x10021a854 <+196>: 0xd63f0100   blr    x8
->  0x10021a858 <+200>: 0x14000006   b      0x10021a870               ; <+224> at kmp_csupport.cpp:360:36

x2 is supposed to contain the last argument, but for some reason it is not pushed. I'm looking at this right now. This one breaks tests on Apple Silicone.

tianshilei1992 added inline comments.Sep 5 2023, 7:29 PM
openmp/runtime/src/kmp_csupport.cpp
356

Sorry, not "pushed to stack". It should be store to x2, based on ARM64 calling convention.