This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Properly set static thread limit (w/o analysis)
ClosedPublic

Authored by jdoerfert on Aug 20 2023, 8:03 PM.

Details

Summary

We used to have two separate implementations to derive the number of
threads used in a target region. This lead us to sometimes miss out on
user provided thread bounds (num_threads, or thread_limit) when we
looked for "constant default values". If we might miss out on the
presence of those bounds, we cannot set the thread_limit statically
since the runtime will try to honor user input rather than cap it at the
"preferred default". This patch replaces the secondary implementation
with the primary in a mode that will not emit code but just look for the
presence, and potentially upper bounds, of thread limiting clauses.

The runtime test would not pass without this rewrite as we missed some
clauses, set the static limit on the device to the preferred value, but
then violated that value at runtime.

Fixes: https://github.com/llvm/llvm-project/issues/64845

Diff Detail

Event Timeline

jdoerfert created this revision.Aug 20 2023, 8:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2023, 8:03 PM
jdoerfert requested review of this revision.Aug 20 2023, 8:03 PM
jhuber6 accepted this revision.Aug 21 2023, 11:49 AM

A few nits. Lots of code but it's mostly just shuffling around stuff that exists. Probably fine given the tests pass.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
6007

Why is this signed but the threads are unsigned now?

6011

The LLVM style uses /*Value=*/ for inline comments https://llvm.org/docs/CodingStandards.html#comment-formatting

6326

Favor C++ style casts?

6329

Should probably be UINT32_MAX

6377

Ditto

This revision is now accepted and ready to land.Aug 21 2023, 11:49 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6008

to have an unsigned value assigned with -1 is really a bad idea

jhuber6 added inline comments.Aug 21 2023, 11:58 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6008

-1 is well defined for unsigned as 0xffffffff, but it's far clearer to use UINT32_MAX like I suggested.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
6008

even ~0U is much better

jdoerfert added inline comments.Aug 21 2023, 12:58 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6007

I did not touch the team stuff yet. Same rewrite is necessary there, but not required right now.

6008

I can do ~0U but the point is really that we look for the -1 later on. I don't do arithmetic, just comparisons.

6326

like reinterpret_cast<uint32_t>(...)? sure.

6329

I figured this way it's more obvious.

jdoerfert marked 8 inline comments as done.Aug 22 2023, 1:53 PM
This revision was landed with ongoing or failed builds.Aug 23 2023, 11:13 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 23 2023, 11:13 AM