[flang][OpenMP] Add OpenMP versions to some tests

[kparzysz]

Some constructs used in the tests are only allowed in certain OpenMP spec versions. Add a flag with the minimum required OpenMP version (other than the default version) to these tests that need it.

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

Some constructs used in the tests are only allowed in certain OpenMP spec versions. Add a flag with the minimum required OpenMP version (other than the default version) to these tests that need it.

---

Full diff: https://github.com/llvm/llvm-project/pull/89295.diff

7 Files Affected:

• (modified) flang/test/Lower/OpenMP/FIR/if-clause.f90 (+4-2)
• (modified) flang/test/Lower/OpenMP/FIR/simd.f90 (+2-1)
• (modified) flang/test/Lower/OpenMP/FIR/target.f90 (+2-1)
• (modified) flang/test/Lower/OpenMP/if-clause.f90 (+4-2)
• (modified) flang/test/Lower/OpenMP/simd.f90 (+3-2)
• (modified) flang/test/Lower/OpenMP/target.f90 (+2-1)
• (modified) flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90 (+3-2)

diff --git a/flang/test/Lower/OpenMP/FIR/if-clause.f90 b/flang/test/Lower/OpenMP/FIR/if-clause.f90
index f686b9708fc54a..683d9f7ef97267 100644
--- a/flang/test/Lower/OpenMP/FIR/if-clause.f90
+++ b/flang/test/Lower/OpenMP/FIR/if-clause.f90
@@ -1,7 +1,9 @@
 ! This test checks lowering of OpenMP IF clauses.
 
-! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck %s
-! RUN: %flang_fc1 -fopenmp -emit-fir %s -o - | FileCheck %s
+! The "if" clause was added to the "simd" directive in OpenMP 5.0, and
+! to the "teams" directive in OpenMP 5.2.
+! RUN: bbc -fopenmp -fopenmp-version=52 -emit-fir %s -o - | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-version=52 -emit-fir %s -o - | FileCheck %s
 
 program main
   integer :: i
diff --git a/flang/test/Lower/OpenMP/FIR/simd.f90 b/flang/test/Lower/OpenMP/FIR/simd.f90
index db7d30295c45d9..91e8750578bfb4 100644
--- a/flang/test/Lower/OpenMP/FIR/simd.f90
+++ b/flang/test/Lower/OpenMP/FIR/simd.f90
@@ -1,6 +1,7 @@
 ! Tests for 2.9.3.1 Simd
 
-! RUN: bbc -fopenmp -emit-fir -hlfir=false %s -o - | FileCheck %s
+! The "if" clause was added to the "simd" directive in OpenMP 5.0.
+! RUN: bbc -fopenmp -fopenmp-version=50 -emit-fir -hlfir=false %s -o - | FileCheck %s
 
 !CHECK-LABEL: func @_QPsimd()
 subroutine simd
diff --git a/flang/test/Lower/OpenMP/FIR/target.f90 b/flang/test/Lower/OpenMP/FIR/target.f90
index ca3162340d7846..d92ba6694de8aa 100644
--- a/flang/test/Lower/OpenMP/FIR/target.f90
+++ b/flang/test/Lower/OpenMP/FIR/target.f90
@@ -1,4 +1,5 @@
-!RUN: %flang_fc1 -emit-fir -flang-deprecated-no-hlfir -fopenmp %s -o - | FileCheck %s
+! The "thread_limit" clause was added to the "target" construct in OpenMP 5.1.
+! RUN: %flang_fc1 -emit-fir -flang-deprecated-no-hlfir -fopenmp -fopenmp-version=51 %s -o - | FileCheck %s
 
 !===============================================================================
 ! Target_Enter Simple
diff --git a/flang/test/Lower/OpenMP/if-clause.f90 b/flang/test/Lower/OpenMP/if-clause.f90
index ce4427a0c2cab2..7c15c275d8cc9d 100644
--- a/flang/test/Lower/OpenMP/if-clause.f90
+++ b/flang/test/Lower/OpenMP/if-clause.f90
@@ -1,7 +1,9 @@
 ! This test checks lowering of OpenMP IF clauses.
 
-! RUN: bbc -fopenmp -emit-hlfir %s -o - | FileCheck %s
-! RUN: %flang_fc1 -fopenmp -emit-hlfir %s -o - | FileCheck %s
+! The "if" clause was added to the "simd" directive in OpenMP 5.0, and
+! to the "teams" directive in OpenMP 5.2.
+! RUN: bbc -fopenmp -fopenmp-version=52 -emit-hlfir %s -o - | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-version=52 -emit-hlfir %s -o - | FileCheck %s
 
 program main
   integer :: i
diff --git a/flang/test/Lower/OpenMP/simd.f90 b/flang/test/Lower/OpenMP/simd.f90
index 190aa615212176..8ec1a3cefb4a60 100644
--- a/flang/test/Lower/OpenMP/simd.f90
+++ b/flang/test/Lower/OpenMP/simd.f90
@@ -1,7 +1,8 @@
 ! Tests for 2.9.3.1 Simd
 
-!RUN: %flang_fc1 -flang-experimental-hlfir -emit-hlfir -fopenmp %s -o - | FileCheck %s
-!RUN: bbc -hlfir -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! The "if" clause was added to the "simd" directive in OpenMP 5.0.
+! RUN: %flang_fc1 -flang-experimental-hlfir -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
+! RUN: bbc -hlfir -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
 
 !CHECK-LABEL: func @_QPsimd()
 subroutine simd
diff --git a/flang/test/Lower/OpenMP/target.f90 b/flang/test/Lower/OpenMP/target.f90
index 51b66327dfb24b..1f7981a8a5ee5b 100644
--- a/flang/test/Lower/OpenMP/target.f90
+++ b/flang/test/Lower/OpenMP/target.f90
@@ -1,4 +1,5 @@
-!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! The "thread_limit" clause was added to the "target" construct in OpenMP 5.1.
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=51 %s -o - | FileCheck %s
 
 !===============================================================================
 ! Target_Enter Simple
diff --git a/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90 b/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90
index d849dd206b9439..90eede4f84108f 100644
--- a/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90
+++ b/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90
@@ -1,5 +1,6 @@
-!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
-!RUN: bbc -emit-hlfir -fopenmp %s -o - | FileCheck %s
+! The "use_device_addr" was added to the "target data" directive in OpenMP 5.0.
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
 
 ! This tests primary goal is to check the promotion of 
 ! non-CPTR arguments from use_device_ptr to 

[tblah]

Maybe we should add this to the newly added openmp flags variable added here #89046

[kparzysz]

Maybe we should add this to the newly added openmp flags variable added here #89046

It's only certain tests that require a specific version. The new variable is set in a lit config file that applies to all (or many) tests. Do you think we should separate tests into different directories based on the minimum required version?

[tblah]

It's only certain tests that require a specific version. The new variable is set in a lit config file that applies to all (or many) tests. Do you think we should separate tests into different directories based on the minimum required version?

I was imagining just running all tests on OpenMP 5.2 then overriding it for any specific tests that need an older version. From my perspective, whichever way leaves fewer "special" tests in the long run would be better because it will make the tests which need a particular configuration stand out while avoiding noise on the others.

Separating into different directories sounds good too but I guess it could be more work to maintain.

I don't feel strongly about which approach is best here.

[kiranchandramohan]

Add a flag with the minimum required OpenMP version (other than the default version) to these tests that need it
-fopenmp-version=52

If I remember correctly, this flag is only used for debug or offload purposes only right? We do not distinguish between openmp versions inside the compiler, right?

[kparzysz]

Add a flag with the minimum required OpenMP version (other than the default version) to these tests that need it
-fopenmp-version=52

If I remember correctly, this flag is only used for debug or offload purposes only right? We do not distinguish between openmp versions inside the compiler, right?

OMP.h.inc defines the function

bool isAllowedClauseForDirective(Directive D, Clause C, unsigned Version);

I use that function to implement the parts of the spec that say something along the lines of "clause applies to the first construct that allows it".

[kiranchandramohan]

Add a flag with the minimum required OpenMP version (other than the default version) to these tests that need it
-fopenmp-version=52

If I remember correctly, this flag is only used for debug or offload purposes only right? We do not distinguish between openmp versions inside the compiler, right?

OMP.h.inc defines the function

bool isAllowedClauseForDirective(Directive D, Clause C, unsigned Version);

I use that function to implement the parts of the spec that say something along the lines of "clause applies to the first construct that allows it".

Would it be OK to assume the latest version i.e. 5.2 or 6 and implement this?
Traditionally, in Flang, there has been no version-specific handling. So this might need a quick discussion with others. Also, what is the scope of the versioning that you are planning? Is it starting from 4.5 or 5.0? Or is it starting from 1.1?

[kparzysz]

Would it be OK to assume the latest version i.e. 5.2 or 6 and implement this? Traditionally, in Flang, there has been no version-specific handling. So this might need a quick discussion with others. Also, what is the scope of the versioning that you are planning? Is it starting from 4.5 or 5.0? Or is it starting from 1.1?

In my current work I take whatever version is present in the MLIR module (which I think is 1.1 by default). The function I mentioned is already there, I'm just calling it. I can use any version with it, I'm not blocked on that.

I was actually thinking about creating an RFC about automatically generating the OMP.td file from the JSON files in the spec. There we'd consume the JSON files from all spec versions, and add version numbers to each directive. We already have versioned clauses, so I thought we could also have versioned directives. Do you think that's worth doing?

[kiranchandramohan]

I was actually thinking about creating an RFC about automatically generating the OMP.td file from the JSON files in the spec. There we'd consume the JSON files from all spec versions, and add version numbers to each directive. We already have versioned clauses, so I thought we could also have versioned directives. Do you think that's worth doing?

I think that is a very interesting idea. Makes sense to me. But this requires a discussion with the wider community.

Either way please do not feel blocked by my comment.

[tblah]

I was actually thinking about creating an RFC about automatically generating the OMP.td file from the JSON files in the spec. There we'd consume the JSON files from all spec versions, and add version numbers to each directive. We already have versioned clauses, so I thought we could also have versioned directives. Do you think that's worth doing?

The RFC sounds interesting.

This patch LGTM anyway. I only wanted to make a suggestion and don't want to block this.

[kparzysz]

Thanks, I'll start working on the RFC then.