[OpenMP] Using SimpleVLA to handle vla usage in ompt-general.cpp.

[DanielCChen]

The openmp runtime failed to build on LoP with LLVM18 on LoP due to the addition of -Wvla-cxx-extension as

llvm-project/openmp/runtime/src/ompt-general.cpp:711:15: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
  711 |   int tmp_ids[ids_size];
      |               ^~~~~~~~
llvm-project/openmp/runtime/src/ompt-general.cpp:711:15: note: function parameter 'ids_size' with unknown value cannot be used in a constant expression
llvm-project/openmp/runtime/src/ompt-general.cpp:704:65: note: declared here
  704 | OMPT_API_ROUTINE int ompt_get_place_proc_ids(int place_num, int ids_size,
      |                                                                 ^
1 error generated.

This patch is to ignore the checking against this usage.

[llvmbot]

@llvm/pr-subscribers-openmp

Author: Daniel Chen (DanielCChen)

Changes

The openmp runtime failed to build on LoP with LLVM18 on LoP due to the addition of -Wvla-cxx-extension as

llvm-project/openmp/runtime/src/ompt-general.cpp:711:15: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
  711 |   int tmp_ids[ids_size];
      |               ^~~~~~~~
llvm-project/openmp/runtime/src/ompt-general.cpp:711:15: note: function parameter 'ids_size' with unknown value cannot be used in a constant expression
llvm-project/openmp/runtime/src/ompt-general.cpp:704:65: note: declared here
  704 | OMPT_API_ROUTINE int ompt_get_place_proc_ids(int place_num, int ids_size,
      |                                                                 ^
1 error generated.

This patch is to ignore the checking against this usage.

---

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

1 Files Affected:

• (modified) openmp/runtime/src/ompt-general.cpp (+3)

diff --git a/openmp/runtime/src/ompt-general.cpp b/openmp/runtime/src/ompt-general.cpp
index 923eea2a563a91..c96bb3153b8a89 100644
--- a/openmp/runtime/src/ompt-general.cpp
+++ b/openmp/runtime/src/ompt-general.cpp
@@ -708,6 +708,9 @@ OMPT_API_ROUTINE int ompt_get_place_proc_ids(int place_num, int ids_size,
   return 0;
 #else
   int i, count;
+#ifdef __clang_major__ >= 18
+#pragma clang diagnostic ignored "-Wvla-cxx-extension"
+#endif
   int tmp_ids[ids_size];
   for (int j = 0; j < ids_size; j++)
     tmp_ids[j] = 0;

[jhuber6]

I would prefer we remove the VLA access, but I don't know how difficult it is for this case.

[shiltian]

No, this is wrong. I previous added a VLA-equal template thing in libomp and replaced all uses there. Can you move it somewhere common such that ompt can also use it?

[DanielCChen]

No, this is wrong. I previous added a VLA-equal template thing in libomp and replaced all uses there. Can you move it somewhere common such that ompt can also use it?

Thanks for the pointer! @shiltian
I am pretty sure this is the change you are referring to (eaab947).

I replaced the offending code in ompt-general.cpp as

//  int tmp_ids[ids_size];
  SimpleVLA<int> tmp_ids(ids_size);

and it fixed the issue.

Since kmp_utils.h only has SimpleVLA, I can either just include it in ompt-general.cpp, or rename it to something like omp_utility.h to be more general. Thoughts?

[shiltian]

I thought ompt source file was in a separate directory. :-)

[shiltian]

put it in the right order?

[DanielCChen]

put it in the right order?

Could you please be more specific? @shiltian

[hubert-reinterpretcast]

@DanielCChen, I think https://llvm.org/docs/CodingStandards.html#include-style says that this header should be moved up. Indeed, it wouldn't fit under the "system include files" comment.

I don't know what to say about the include of ompt-specific.cpp (yes, .cpp!) below.

[shiltian]

Yes. That is what I meant.

I don't know what to say about the include of ompt-specific.cpp (yes, .cpp!) below.

Me neither. ;-) That is before libomp was contributed to LLVM.

[DanielCChen]

Thanks. I will move it before the system #include then.