Page MenuHomePhabricator

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

Authored by DavidTruby on Tue, Nov 22, 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

Unit TestsFailed

TimeTest
0 msx64 debian > LLVM-Unit.Frontend/_/LLVMFrontendTests/OpenMPIRBuilderTest::ParallelIfCond
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/unittests/Frontend/./LLVMFrontendTests --gtest_filter=OpenMPIRBuilderTest.ParallelIfCond
60,040 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp

Event Timeline

DavidTruby created this revision.Tue, Nov 22, 6:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby requested review of this revision.Tue, Nov 22, 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.Tue, Nov 22, 8:34 AM

Nice. Thanks for fixing this the right way.

This revision is now accepted and ready to land.Tue, Nov 22, 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
967

Does this need an update since there is no ThenBB?

1030

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?