[Flang][FlangRT][Runtime] Add RT_OFFLOAD_API_GROUP_BEGIN to missing symbols on AMDGPU

[agozillon]

After the recent move to work queues, in certain cases when linking in the fortran runtime built for offload on AMDGPU as required in certain cases, we'll get missing symbols when linking. This PR tries to address this issue by encompassing more of the library in RT_OFFLOAD_API_GROUP_BEGIN, which has the affect of compiling these functions for AMDGPU, resolving the missing symbols.

This PR should address the following issue: #145888

[agozillon]

There's possibly a less intrusive/simpler way of resolving this, if so please don't hesitate to suggest it! :-)

[vzakhari]

Thank you for the changes!

I wish clang supported declare target properly for the explicit instantiations and did not require declare target for the class declaration. Since the function references are automatically declare target if they are referenced by other declare target functions according to OpenMP standard, it really does not make much sense to require declare target to be applied to the class declaration and its methods' declarations.

I guess a less intrusive way would be to expand RT_API_ATTRS into something like [[omp::declare_target]], but this is not part of OpenMP standard :)

[vzakhari]

If I understand correctly, this is related to #145888 - can you please add a reference into the description?

[agozillon]

Thank you for the changes!

I wish clang supported declare target properly for the explicit instantiations and did not require declare target for the class declaration. Since the function references are automatically declare target if they are referenced by other declare target functions according to OpenMP standard, it really does not make much sense to require declare target to be applied to the class declaration and its methods' declarations.

Yes, I actually thought we already had the implicit marking in Clang C++/OpenMP, but it seems to not be the case, or at least it doesn't handle more complicated cases unfortunately! :-) So sadly need to be a bit more generous with the RT_OFFLOAD_API_GROUP_BEGIN macros, which isn't the prettiest or best thing for compile times long term I imagine.

I guess a less intrusive way would be to expand RT_API_ATTRS into something like [[omp::declare_target]], but this is not part of OpenMP standard :)

I'd personally prefer to keep it as close to OpenMP standard acceptable as possible, mainly so other OpenMP compilers can come along and compile the runtime if they'd like to (I do think OpenMP does support the attribute specifier syntax in the more recent standard, but I may be misremembering) ! But happy to make this change if you feel it'd be better! :-)

[agozillon]

If I understand correctly, this is related to #145888 - can you please add a reference into the description?

I didn't actually notice this issue, I ran into the problem separately in some map tests I have! Thank you for pointing me to it, yes, I do believe this would fix that issue, at least the symbols referenced look similar, if it doesn't fix them all I'd be happy to create a subsequent PR to fix the rest. I'll alter the description just now to add the issue, thank you again :-)!

[vzakhari]

I'd personally prefer to keep it as close to OpenMP standard acceptable as possible

I second this. Let's keep this PR as-is. It would be worth "fixing" clang to apply declare target to explicit instantiations, but I am not sure who can do this.

[agozillon]

I'd personally prefer to keep it as close to OpenMP standard acceptable as possible

I second this. Let's keep this PR as-is.

Sounds good! :-)

It would be worth "fixing" clang to apply declare target to explicit instantiations, but I am not sure who can do this.

It would indeed, I am unfortunately not the best person to do it I think unfortunately, aware of how it works in Flang, but not Clang as of yet... but I can bring it up internally and maybe someone will have the bandwidth to look at it.

Thank you very much for the review @vzakhari