[Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGPU modifications #152631

agozillon · 2025-08-08T03:11:01Z

We have some modifications downstream to compile the flang runtime for amdgpu using clang OpenMP, some more hacky than others to workaround (hopefully temporary) compiler issues. The additions here are the non-hacky alterations.

Main changes:
* Create freestanding versions of memcpy, strlen and memmove, and replace std:: references with these so that we can default to std:: when it's available, or our own Flang implementation when it's not. * Wrap more bits and pieces of the library in declare target wrappers (RT_* macros). * Fix some warnings that'll pose issues with werror on, in this case having the namespace infront of variables passed to templates.

Another minor issues that'll likely still pop up depending on the program you're linking with is that abort will be undefined, it is perhaps possible to solve it with a freestanding implementation as with memcpy etc. but we end up with multiple definitions in this case. An alternative is to create an empty extern "c" version (which can be empty or forwrd on to the builtin).

Co-author: Dan Palermo Dan.Palermo@amd.com

github-actions · 2025-08-08T03:13:26Z

✅ With the latest revision this PR passed the C/C++ code formatter.

klausler · 2025-08-08T15:43:25Z

flang-rt/include/flang-rt/runtime/work-queue.h

@@ -127,8 +127,8 @@ class Elementwise {
  const Descriptor &instance_, *from_{nullptr};
  std::size_t elements_{instance_.InlineElements()};
  std::size_t elementAt_{0};
-  SubscriptValue subscripts_[common::maxRank];
-  SubscriptValue fromSubscripts_[common::maxRank];
+  SubscriptValue subscripts_[maxRank];


I can remove it if you wish, but this flags up as a warning in Clang I believe, that'll cause us to error out in with -werror @dpalermo will know a bit more about this alteration I think :-)

What is the warning from clang?

I might be misremembering the reason, it's been a while, I've checked and it's no longer a problem, so I'm happy to revert this :-)

klausler · 2025-08-08T15:44:50Z

flang-rt/include/flang-rt/runtime/buffer.h

@@ -158,8 +158,8 @@ template <typename STORE, std::size_t minBuffer = 65536> class FileFrame {
      // Avoid passing a null pointer, since it would result in an undefined
      // behavior.
      if (old != nullptr) {
-        std::memcpy(buffer_, old + start_, chunk);
-        std::memcpy(buffer_ + chunk, old, length_ - chunk);
+        Fortran::runtime::memcpy(buffer_, old + start_, chunk);


You don't need the Fortran prefix, and using it makes this code inconsistent with other namespace usage in the runtime.

Sounds good, will make the alteration in the next update after the other discussions are resolved

klausler · 2025-08-08T15:46:47Z

flang-rt/lib/runtime/io-api-minimal.cpp

@@ -161,4 +161,4 @@ void std::__libcpp_verbose_abort(char const *format, ...) noexcept(
 }
 #endif

-RT_EXT_API_GROUP_END
+RT_EXT_API_GROUP_END


I think you just deleted the terminal newline here.

Nice catch, thank you! Will re-add in subsequent update.

klausler · 2025-08-08T15:50:25Z

flang/include/flang/Runtime/freestanding-tools.h

@@ -23,6 +23,16 @@
 #define STD_FILL_N_UNSUPPORTED 1
 #endif

+#if !defined(STD_MEMSET_UNSUPPORTED) && \
+    (defined(__CUDACC__) || defined(__CUDA__)) && defined(__CUDA_ARCH__)
+#define STD_MEMSET_UNSUPPORTED 1


Won't this change existing compilations that work with CUDA?

Happy to remove it if we wish, it was to maintain consistency with the rest of the CUDA support below (e.g. memmove) when we added the other bits and pieces, @vzakhari may know better if this is a reasonable addition or not or you of course might as well :-) so please do advise if either of you wish it removed or altered, happy to do so!

I believe std::memset and std::memcpy are supported by at least nvcc, so I would prefer these additions to be removed.

Sounds good, will do so on the next pass!

vzakhari · 2025-08-08T16:11:09Z

flang/include/flang/Runtime/freestanding-tools.h

@@ -63,6 +73,24 @@
 #define STD_TOUPPER_UNSUPPORTED 1
 #endif

+#if defined(OMP_OFFLOAD_BUILD) || defined(OMP_NOHOST_BUILD)


Is it possible to add this only for the particular OpenMP compiler you are using currently? I think some other OpenMP compilers may support some of these STD methods, so I would prefer not to disable them all for all compilers that may be used for out-of-tree flang-rt builds.

In addition, I think we should not do this for the host part of the offload build.

Is it possible to add this only for the particular OpenMP compiler you are using currently? I think some other OpenMP compilers may support some of these STD methods, so I would prefer not to disable them all for all compilers that may be used for out-of-tree flang-rt builds.

That makes sense, I believe it should be possible, I think Clang (which is what we use) should have an environment variable specified during compilation to notify what compiler is in use, so should be able to use that if I'm remembering correctly.

In addition, I think we should not do this for the host part of the offload build.

Makes sense as well, I'll change the || -> && and it should only be applicable to the offload build and fall back on the std or builtin implementations.

agozillon

Added some replies inline, I'll update the PR once the discussions have been concluded!

agozillon · 2025-08-08T16:36:10Z

flang-rt/include/flang-rt/runtime/work-queue.h

@@ -127,8 +127,8 @@ class Elementwise {
  const Descriptor &instance_, *from_{nullptr};
  std::size_t elements_{instance_.InlineElements()};
  std::size_t elementAt_{0};
-  SubscriptValue subscripts_[common::maxRank];
-  SubscriptValue fromSubscripts_[common::maxRank];
+  SubscriptValue subscripts_[maxRank];


I can remove it if you wish, but this flags up as a warning in Clang I believe, that'll cause us to error out in with -werror @dpalermo will know a bit more about this alteration I think :-)

agozillon · 2025-08-08T16:37:01Z

flang-rt/include/flang-rt/runtime/buffer.h

@@ -158,8 +158,8 @@ template <typename STORE, std::size_t minBuffer = 65536> class FileFrame {
      // Avoid passing a null pointer, since it would result in an undefined
      // behavior.
      if (old != nullptr) {
-        std::memcpy(buffer_, old + start_, chunk);
-        std::memcpy(buffer_ + chunk, old, length_ - chunk);
+        Fortran::runtime::memcpy(buffer_, old + start_, chunk);


Sounds good, will make the alteration in the next update after the other discussions are resolved

agozillon · 2025-08-08T16:37:34Z

flang-rt/lib/runtime/io-api-minimal.cpp

@@ -161,4 +161,4 @@ void std::__libcpp_verbose_abort(char const *format, ...) noexcept(
 }
 #endif

-RT_EXT_API_GROUP_END
+RT_EXT_API_GROUP_END


Nice catch, thank you! Will re-add in subsequent update.

agozillon · 2025-08-08T16:41:56Z

flang/include/flang/Runtime/freestanding-tools.h

@@ -23,6 +23,16 @@
 #define STD_FILL_N_UNSUPPORTED 1
 #endif

+#if !defined(STD_MEMSET_UNSUPPORTED) && \
+    (defined(__CUDACC__) || defined(__CUDA__)) && defined(__CUDA_ARCH__)
+#define STD_MEMSET_UNSUPPORTED 1


Happy to remove it if we wish, it was to maintain consistency with the rest of the CUDA support below (e.g. memmove) when we added the other bits and pieces, @vzakhari may know better if this is a reasonable addition or not or you of course might as well :-) so please do advise if either of you wish it removed or altered, happy to do so!

agozillon · 2025-08-08T16:46:30Z

flang/include/flang/Runtime/freestanding-tools.h

@@ -63,6 +73,24 @@
 #define STD_TOUPPER_UNSUPPORTED 1
 #endif

+#if defined(OMP_OFFLOAD_BUILD) || defined(OMP_NOHOST_BUILD)


Is it possible to add this only for the particular OpenMP compiler you are using currently? I think some other OpenMP compilers may support some of these STD methods, so I would prefer not to disable them all for all compilers that may be used for out-of-tree flang-rt builds.

That makes sense, I believe it should be possible, I think Clang (which is what we use) should have an environment variable specified during compilation to notify what compiler is in use, so should be able to use that if I'm remembering correctly.

In addition, I think we should not do this for the host part of the offload build.

Makes sense as well, I'll change the || -> && and it should only be applicable to the offload build and fall back on the std or builtin implementations.

agozillon · 2025-08-11T17:36:20Z

Attempted to address all the feedback in the last commit (plus force pushed once more to fix some formatting)

klausler

Are there other APIs from std:: in the runtime that should be replaced?

klausler · 2025-08-11T17:39:28Z

flang-rt/lib/runtime/assign.cpp

@@ -15,6 +15,7 @@
 #include "flang-rt/runtime/tools.h"
 #include "flang-rt/runtime/type-info.h"
 #include "flang-rt/runtime/work-queue.h"
+#include "flang/Runtime/stop.h"


why is this additional include directive necessary?

Will double check if the includes are necessary!

No requirement for this one anymore, thank you for pointing it out!

klausler · 2025-08-11T17:40:41Z

flang-rt/lib/runtime/copy.cpp

@@ -10,6 +10,7 @@
 #include "stack.h"
 #include "flang-rt/runtime/descriptor.h"
 #include "flang-rt/runtime/terminator.h"
+#include "flang-rt/runtime/tools.h"


is this additional include directive necessary?

in this case it is unfortunately, due needing the memcpy from the fortran::runtime.

But memcpy is defined in freestanding-tools.h, no?

Good point, thank you! I've replaced tools with freestanding-tools.h in the most recent update.

agozillon · 2025-08-11T17:54:21Z

Are there other APIs from std:: in the runtime that should be replaced?

Not that we've found so far, but there will likely be others as the runtime gets built on! So we're happy to address them as they come up.

There is an issue with abort (C abort I think, rather than std::) currently, but the method we use to work around it downstream at the moment isn't particularly upstreamable, we create an local (to the .cpp source file with the issue) extern "C" function in-place of abort to forward to the fortran runtime's abort. Unfortunately, couldn't replicate similar functionality to the memset/memcpy in the PR as we end up with duplicate symbols.

agozillon · 2025-08-11T18:40:46Z

Checked the pointed out includes in the last comments and removed one in the last commit, the other is required for the fortran::runtime::memcpy functionality it seems.

agozillon · 2025-08-14T15:04:46Z

Updated the PR stack, the main addition was new RT_OFFLOAD_API_GROUP wrappers in temporary-stack.cpp.

But otherwise I rebased and reformatted the PR to fix the code formatting!

But the PR should address all current comments and pass the CI now, please do feel free to add more comments however if you wish anything further changed! :-)

agozillon · 2025-08-19T10:28:02Z

Small ping for further review or approvals if possible please :-)

klausler · 2025-08-19T14:35:18Z

flang-rt/lib/runtime/extensions.cpp

@@ -204,8 +204,8 @@ void FORTRAN_PROCEDURE_NAME(getarg)(
 void FORTRAN_PROCEDURE_NAME(getlog)(char *arg, std::int64_t length) {
 #if _REENTRANT || _POSIX_C_SOURCE >= 199506L
  if (length >= 1 && getlogin_r(arg, length) == 0) {
-    auto loginLen{std::strlen(arg)};
-    std::memset(
+    auto loginLen{Fortran::runtime::strlen(arg)};


Just runtime:: is sufficient.

Went back over these to double check and removed Fortran:: from this one, the others appear to need the namespace prefix at the moment or get undeclared identifier errors.

Why? The identifiers are declared in the runtime namespace, no? Maybe you're missing an include or something.

Possible and I'm happy to investigate a bit, just assumed it's perhaps intended for those files as we do use Fortran::runtime:: elsewhere in it!

I think I managed to get to the bottom of it, they reside outside of a namespace or in an anonymous namespace, so a using namespace Fortran is required. Should be addressed in the latest commit.

klausler · 2025-08-19T14:35:54Z

flang-rt/lib/runtime/temporary-stack.cpp

@@ -97,7 +99,7 @@ void DescriptorStorage<COPY_VALUES>::resize(size_type newCapacity) {
  // Avoid passing a null pointer, since it would result in an undefined
  // behavior.
  if (data_ != nullptr) {
-    memcpy(newData, data_, capacity_ * sizeof(Descriptor *));
+    Fortran::runtime::memcpy(newData, data_, capacity_ * sizeof(Descriptor *));


Just runtime:: suffices.

klausler · 2025-08-19T14:36:22Z

flang-rt/lib/runtime/time-intrinsic.cpp

@@ -276,13 +276,13 @@ static void DateAndTimeUnavailable(Fortran::runtime::Terminator &terminator,
    char *zone, std::size_t zoneChars,
    const Fortran::runtime::Descriptor *values) {
  if (date) {
-    std::memset(date, static_cast<int>(' '), dateChars);
+    Fortran::runtime::memset(date, static_cast<int>(' '), dateChars);


Just runtime:: is needed here and in the following lines.

I believe it is necessary to have the Fortran:: prefix in the remaining locations, it incurs a compile time error otherwise. And the Fortran:: prefix is used elsewhere in these files e.g. one of the input parameter in the above case.

really? what happens?

klausler · 2025-08-19T14:38:00Z

flang/include/flang/Runtime/freestanding-tools.h

+  while (count--) {
+    *to++ = *from++;
+  }
+  return;


You don't need a return; statement before the closing brace of a function.

Sounds good! Happy to adjust.

agozillon · 2025-08-19T15:16:43Z

rebased on top of main in the last PR and amended the requested changes into the last commit so had to do a force push, so my appologies for that.

The only additional changes should be the remove of the requested return; and the removal of one more Fortran:: namespace prefix, the remaining 2 require it at the moment it seems.

agozillon · 2025-08-19T16:30:07Z

Last PR should address all the comments currently. I'll git squash and clang-format to make the CI happy once we're happy with the PR (so as to make reviewing any changes easier for the time being).

agozillon · 2025-08-27T14:16:57Z

Small ping for reviewer attention on this please :-)

vzakhari

LGTM, with one minor comment. Thank you!

vzakhari · 2025-08-27T15:11:48Z

flang-rt/lib/runtime/time-intrinsic.cpp

@@ -420,7 +423,7 @@ static void GetDateAndTime(Fortran::runtime::Terminator &terminator, char *date,
  auto copyBufferAndPad{
      [&](char *dest, std::size_t destChars, std::size_t len) {
        auto copyLen{std::min(len, destChars)};
-        std::memcpy(dest, buffer, copyLen);
+        Fortran::runtime::memcpy(dest, buffer, copyLen);


Suggested change

Fortran::runtime::memcpy(dest, buffer, copyLen);

runtime::memcpy(dest, buffer, copyLen);

agozillon · 2025-08-27T18:06:35Z

Thank you both very much! I'll address the last comment, squash and clang-format and land the PR in the next couple of days unless there's any further comments in between :-)

We have some modifications downstream to compile the flang runtime for amdgpu using clang OpenMP, some more hacky than others to workaround (hopefully temporary) compiler issues. The additions here are the non-hacky alterations. Main changes: * Create freestanding versions of memcpy, strlen and memmove, and replace std:: references with these so that we can default to std:: when it's available, or our own Flang implementation when it's not. * Wrap more bits and pieces of the library in declare target wrappers (RT_* macros). * Fix some warnings that'll pose issues with werror on, in this case having the namespace infront of variables passed to templates. Another minor issues that'll likely still pop up depending on the program you're linking with is that abort will be undefined, it is perhaps possible to solve it with a freestanding implementation as with memcpy etc. but we end up with multiple definitions in this case. An alternative is to create an empty extern "c" version (which can be empty or forwrd on to the builtin). Co-author: Dan Palermo Dan.Palermo@amd.com

…cations (llvm#152631) We have some modifications downstream to compile the flang runtime for amdgpu using clang OpenMP, some more hacky than others to workaround (hopefully temporary) compiler issues. The additions here are the non-hacky alterations. Main changes: * Create freestanding versions of memcpy, strlen and memmove, and replace std:: references with these so that we can default to std:: when it's available, or our own Flang implementation when it's not. * Wrap more bits and pieces of the library in declare target wrappers (RT_* macros). * Fix some warnings that'll pose issues with werror on, in this case having the namespace infront of variables passed to templates. Another minor issues that'll likely still pop up depending on the program you're linking with is that abort will be undefined, it is perhaps possible to solve it with a freestanding implementation as with memcpy etc. but we end up with multiple definitions in this case. An alternative is to create an empty extern "c" version (which can be empty or forwrd on to the builtin). Co-author: Dan Palermo Dan.Palermo@amd.com

…U modifications (llvm#152631)" This reverts commit 30d2cb5.

…cations (llvm#152631) We have some modifications downstream to compile the flang runtime for amdgpu using clang OpenMP, some more hacky than others to workaround (hopefully temporary) compiler issues. The additions here are the non-hacky alterations. Main changes: * Create freestanding versions of memcpy, strlen and memmove, and replace std:: references with these so that we can default to std:: when it's available, or our own Flang implementation when it's not. * Wrap more bits and pieces of the library in declare target wrappers (RT_* macros). * Fix some warnings that'll pose issues with werror on, in this case having the namespace infront of variables passed to templates. Another minor issues that'll likely still pop up depending on the program you're linking with is that abort will be undefined, it is perhaps possible to solve it with a freestanding implementation as with memcpy etc. but we end up with multiple definitions in this case. An alternative is to create an empty extern "c" version (which can be empty or forwrd on to the builtin). Co-author: Dan Palermo Dan.Palermo@amd.com

agozillon requested review from dpalermo, klausler and vzakhari August 8, 2025 03:11

llvmbot added the flang Flang issues not falling into any other category label Aug 8, 2025

agozillon force-pushed the flang-runtime-tweaks-for-amd-offload branch from a3304f0 to cc89cea Compare August 8, 2025 03:17

klausler reviewed Aug 8, 2025

View reviewed changes

vzakhari reviewed Aug 8, 2025

View reviewed changes

agozillon commented Aug 8, 2025

View reviewed changes

agozillon force-pushed the flang-runtime-tweaks-for-amd-offload branch from fc16dff to be67c22 Compare August 11, 2025 17:34

klausler reviewed Aug 11, 2025

View reviewed changes

agozillon force-pushed the flang-runtime-tweaks-for-amd-offload branch from 3353c60 to 0416575 Compare August 14, 2025 15:00

klausler reviewed Aug 19, 2025

View reviewed changes

klausler requested review from klausler and removed request for klausler August 19, 2025 14:38

agozillon force-pushed the flang-runtime-tweaks-for-amd-offload branch from 0416575 to de94a2c Compare August 19, 2025 15:12

klausler requested review from klausler and removed request for klausler August 19, 2025 15:30

vzakhari approved these changes Aug 27, 2025

View reviewed changes

agozillon force-pushed the flang-runtime-tweaks-for-amd-offload branch from 0e5ef24 to e6b83ed Compare August 29, 2025 20:33

agozillon changed the title ~~[Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGPU~~ [Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGPU modifications Aug 29, 2025

agozillon merged commit 30d2cb5 into llvm:main Aug 29, 2025
9 checks passed

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Aug 30, 2025

Revert "[Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGP…

dc23265

…U modifications (llvm#152631)" This reverts commit 30d2cb5.

	Fortran::runtime::memcpy(dest, buffer, copyLen);
	runtime::memcpy(dest, buffer, copyLen);

[Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGPU modifications #152631

[Flang][OpenMP][Runtime] Minor Flang runtime for OpenMP AMDGPU modifications #152631

Uh oh!

Conversation

agozillon commented Aug 8, 2025

Uh oh!

github-actions bot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agozillon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agozillon commented Aug 11, 2025

Uh oh!

klausler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agozillon commented Aug 11, 2025

Uh oh!

agozillon commented Aug 11, 2025

Uh oh!

agozillon commented Aug 14, 2025

Uh oh!

agozillon commented Aug 19, 2025

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Aug 8, 2025 •

edited

Loading

agozillon Aug 19, 2025 •

edited

Loading