Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[OpenMP] Metadirective uses default for non-const user condition #82754

Open
Thyre opened this issue Feb 23, 2024 · 9 comments · May be fixed by #86457
Open

[OpenMP] Metadirective uses default for non-const user condition #82754

Thyre opened this issue Feb 23, 2024 · 9 comments · May be fixed by #86457
Labels
clang:openmp OpenMP related changes to Clang good first issue https://github.com/llvm/llvm-project/contribute openmp

Comments

@Thyre
Copy link

Thyre commented Feb 23, 2024

Description

I've been looking at a few lesser used OpenMP directives recently and ran across metadirective. While the syntax always looks a bit intimidating, I wanted to know more about it and experimented with a few test cases. While doing this, I've noticed that Clang seems to use the default case whenever a user condition is used. To show this issue, I've prepared the following reproducer.

Reproducer

#include <cassert>
#include <iostream>
#include <omp.h>

int main( int argc, char** argv )
{
    constexpr size_t num_threads = 4;

    constexpr int constexpr_val = 1;
    #pragma omp metadirective \
        when( user= {condition(constexpr_val > 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }

    const int const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(const_val > 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );           
        }     

    int non_const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(non_const_val > 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }     
}

Here, I just want to make sure that the metadirective works. In theory, all three cases should result in the same: Having a parallel region with four threads. However, this is not the case with LLVM 15.0.7 and newer. Here's the output from LLVM trunk:

$ clang --version
clang version 19.0.0git (https://github.com/llvm/llvm-project.git 1fe6be8794964c011aeba7a66bd2dcd891d21ab0)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/apps/software/Clang/trunk/bin
$ clang++ -fopenmp reproducer.cpp
$ ./a.out                                                                                                                                                                                                                                                          
a.out: reproducer.cpp:33: int main(int, char **): Assertion `num_threads == omp_get_num_threads()' failed.
[1]    1133016 IOT instruction (core dumped)  ./a.out

NVHPC 23.9 handles this test perfectly fine. oneAPI 2024.0 fails the same way and GCC 12.3.0 even fails the first assertion. Replacing the non_const_val with a true user condition (like std::cin > non_const_val) doesn't change the outcome of the test.
Looking at the IR, it seems like the metadirective is replaced by the default, resulting in no check at all if the parallel region should be used: https://godbolt.org/z/9oM5ooeE1

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 23, 2024

@llvm/issue-subscribers-openmp

Author: Jan André Reuter (Thyre)

### Description

I've been looking at a few lesser used OpenMP directives recently and ran across metadirective. While the syntax always looks a bit intimidating, I wanted to know more about it and experimented with a few test cases. While doing this, I've noticed that Clang seems to use the default case whenever a user condition is used. To show this issue, I've prepared the following reproducer.

Reproducer

#include &lt;cassert&gt;
#include &lt;iostream&gt;
#include &lt;omp.h&gt;

int main( int argc, char** argv )
{
    constexpr size_t num_threads = 4;

    constexpr int constexpr_val = 1;
    #pragma omp metadirective \
        when( user= {condition(constexpr_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }

    const int const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(const_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );           
        }     

    int non_const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(non_const_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }     
}

Here, I just want to make sure that the metadirective works. In theory, all three cases should result in the same: Having a parallel region with four threads. However, this is not the case with LLVM 15.0.7 and newer. Here's the output from LLVM trunk:

$ clang --version
clang version 19.0.0git (https://github.com/llvm/llvm-project.git 1fe6be8794964c011aeba7a66bd2dcd891d21ab0)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/apps/software/Clang/trunk/bin
$ clang++ -fopenmp reproducer.cpp
$ ./a.out                                                                                                                                                                                                                                                          
a.out: reproducer.cpp:33: int main(int, char **): Assertion `num_threads == omp_get_num_threads()' failed.
[1]    1133016 IOT instruction (core dumped)  ./a.out

NVHPC 23.9 handles this test perfectly fine. oneAPI 2024.0 fails the same way and GCC 12.3.0 even fails the first assertion. Replacing the non_const_val with a true user condition (like std::cin &gt; non_const_val) doesn't change the outcome of the test.
Looking at the IR, it seems like the metadirective is replaced by the default, resulting in no check at all if the parallel region should be used: https://godbolt.org/z/9oM5ooeE1

@shiltian shiltian added clang:openmp OpenMP related changes to Clang good first issue https://github.com/llvm/llvm-project/contribute labels Feb 25, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 25, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 25, 2024

@llvm/issue-subscribers-good-first-issue

Author: Jan André Reuter (Thyre)

### Description

I've been looking at a few lesser used OpenMP directives recently and ran across metadirective. While the syntax always looks a bit intimidating, I wanted to know more about it and experimented with a few test cases. While doing this, I've noticed that Clang seems to use the default case whenever a user condition is used. To show this issue, I've prepared the following reproducer.

Reproducer

#include &lt;cassert&gt;
#include &lt;iostream&gt;
#include &lt;omp.h&gt;

int main( int argc, char** argv )
{
    constexpr size_t num_threads = 4;

    constexpr int constexpr_val = 1;
    #pragma omp metadirective \
        when( user= {condition(constexpr_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }

    const int const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(const_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );           
        }     

    int non_const_val = 1;
    #pragma omp metadirective \
        when( user= {condition(non_const_val &gt; 0)} : parallel num_threads( num_threads ) ) \
        default()
        {
            #pragma omp single
            assert( num_threads == omp_get_num_threads() );
        }     
}

Here, I just want to make sure that the metadirective works. In theory, all three cases should result in the same: Having a parallel region with four threads. However, this is not the case with LLVM 15.0.7 and newer. Here's the output from LLVM trunk:

$ clang --version
clang version 19.0.0git (https://github.com/llvm/llvm-project.git 1fe6be8794964c011aeba7a66bd2dcd891d21ab0)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/apps/software/Clang/trunk/bin
$ clang++ -fopenmp reproducer.cpp
$ ./a.out                                                                                                                                                                                                                                                          
a.out: reproducer.cpp:33: int main(int, char **): Assertion `num_threads == omp_get_num_threads()' failed.
[1]    1133016 IOT instruction (core dumped)  ./a.out

NVHPC 23.9 handles this test perfectly fine. oneAPI 2024.0 fails the same way and GCC 12.3.0 even fails the first assertion. Replacing the non_const_val with a true user condition (like std::cin &gt; non_const_val) doesn't change the outcome of the test.
Looking at the IR, it seems like the metadirective is replaced by the default, resulting in no check at all if the parallel region should be used: https://godbolt.org/z/9oM5ooeE1

@rufevean
Copy link

rufevean commented Feb 26, 2024

i wanted to work on this issue but when am trying to reproduce the issue, its showing

rep.cpp:3:10: fatal error: 'omp.h' file not found
    3 | #include <omp.h>
      |          ^~~~~~~
1 error generated.

anyway i can solve this?

@Thyre
Copy link
Author

Thyre commented Feb 26, 2024

i wanted to work on this issue but when am trying to reproduce the issue, its showing

rep.cpp:3:10: fatal error: 'omp.h' file not found
    3 | #include <omp.h>
      |          ^~~~~~~
1 error generated.

anyway i can solve this?

It looks like your compiler does not find the OpenMP headers. If you compiled LLVM with OpenMP enabled (-DLLVM_ENABLE_RUNTIMES="openmp"), the header can be found at ${CMAKE_INSTALL_PREFIX}/./lib/clang/19/include/omp.h with 19 representing the major version number (17 for 17.0.6, 18 for 18.1.0-rc[x] and so on).

Make sure that your compiler does find the header, either by adding -I[path-to-include-directory] or setting CPATH. Normally, clang should be able to find the headers and libraries out of the box though.

@rufevean
Copy link

rufevean commented Feb 27, 2024

okay i think the error is from

llvm-project/clang/lib/Sema/SemaOpenMp.cpp

Am i going on the right path? or am i missing something?

@Sh0g0-1758
Copy link
Contributor

Sh0g0-1758 commented Feb 29, 2024

When I try to build llvm using the following command :

cmake -DCMAKE_INSTALL_PREFIX=`pwd`/inst -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES="openmp" -DCMAKE_BUILD_TYPE=Release ../llvm

I get the following error :

[ 96%] Performing build step for 'runtimes'
[  0%] Built target libomp-needed-headers
[  0%] Building CXX object openmp/runtime/src/CMakeFiles/omp.dir/kmp_alloc.cpp.o
In file included from /home/shogo/master/dev/low/llvm-project/openmp/runtime/src/kmp_alloc.cpp:13:
/home/shogo/master/dev/low/llvm-project/openmp/runtime/src/kmp.h:80:10: fatal error: 'limits' file not found
   80 | #include <limits>
      |          ^~~~~~~~
1 error generated.
make[5]: *** [openmp/runtime/src/CMakeFiles/omp.dir/build.make:76: openmp/runtime/src/CMakeFiles/omp.dir/kmp_alloc.cpp.o] Error 1
make[4]: *** [CMakeFiles/Makefile2:1043: openmp/runtime/src/CMakeFiles/omp.dir/all] Error 2
make[3]: *** [Makefile:136: all] Error 2
make[2]: *** [runtimes/CMakeFiles/runtimes.dir/build.make:89: runtimes/runtimes-stamps/runtimes-build] Error 2
make[1]: *** [CMakeFiles/Makefile2:110882: runtimes/CMakeFiles/runtimes.dir/all] Error 2
make: *** [Makefile:156: all] Error 2

Any idea what am I doing wrong ?

@shiltian
Copy link
Contributor

@Sh0g0-1758 It looks like your C++ compiler can't find the header. Not sure what can cause this.

@shiltian
Copy link
Contributor

okay i think the error is from

llvm-project/clang/lib/Sema/SemaOpenMp.cpp

Am i going on the right path? or am i missing something?

It sounds reasonable. Feel free to post a PR along with a test case and we can start from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:openmp OpenMP related changes to Clang good first issue https://github.com/llvm/llvm-project/contribute openmp
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants