Update: Removed statics (libomptarget->plugins). Style.

Update + rebase to reflect the landing of https://reviews.llvm.org/D124070

In D124070#4320259, @tianshilei1992 wrote:

I think the patch looks good, though I didn't test it yet. I don't want to block here. Go for it. :-)

@tianshilei1992 If it is not too much to ask for: Could you please build the current patch with your configuration and let me know if there were issues?

Added conditional linking of OMPT to PluginInterface as well as amdgpu and cuda plugins`.

If there are no further problems or requests, I would like to re-land this revision.

Made #ifdef span OmptCallback.cpp to avoid build errors when OMPT is disabled.
Added dummy definition for exported symbol ompt_libomptarget_connect.

In D124070#4313476, @tianshilei1992 wrote:

Either only adding the source file when OMPT is enabled, or as suggested by @dhruvachak, guarding the entire file with the OMPT_SUPPORT.

@tianshilei1992 Thank you very much!
Sorry that I was unavailable for the last hour, just got at the PC.

Small fix w.r.t. debug builds.

Addressed style issues, dreaded whitespace & removed DEBUG_PREFIX testing

Updated to accomodate the changes made in the previous patch.

Implemented further feedback (removed statics & added corresponding assert)
edit: Please note that the namespace feedback has been addressed in the "next" patch in this stack: D12465

Removed debug prints / comments and fixed a problem when no OMPT was initialized.

Rebase + implementation of feedback

Corrected commit (apologies)

Rebase

In D147511#4280795, @tianshilei1992 wrote:

I didn't see that is the case. https://godbolt.org/z/TdcfxfEej I didn't set the thread limit and then it is 0.
The spec says thread_limit has to be greater than zero. So here using 0 to indicate the clause is not set looks good to me.

Feedback should be addressed from my perspective -- is there anything left to do?
Thanks in advance!

Rebase + clarifications on this patch's intent

Refactored the fix such that it covers not only the '-1' case, but all negative integers.
Added a testcase, also to demonstrate the issue.

As far as I know: No, this patch should avoid exactly that case. (It seems that param would be set to -1 when there's no thread_limit clause, which caused problems.)

Patch rework; implemented feedback.

Patch rework, implementing the mentioned changes.

In D145591#4182360, @jhuber6 wrote:

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

When more changes have accumulated I'll update this diff with the changes I replied to with 'Done'.

As discussed with Dhruva.

As discussed with Dhruva.

As discussed with Dhruva.

Refactored patch for use with nextgen plugins.

Refactored patch for use with nextgen plugins.

Refactored patch for use with nextgen plugins.

Refactored patch for use with nextgen plugins.

In D145591#4179144, @tra wrote:

How is this different from compiling a C++ file with opemnp directives in it? AFAICT neither clang nor gcc issue anywarnings: https://godbolt.org/z/5Me3dnTdr

What makes the warnings necessary for HIP?

Fixed update.

Removed dependency on previous patches.
Note: Tests will now check for failed callback registrations.

Rebased

Rebased

Rebased + spelling fixes

Rebased + related fixes

Fixed wrong diff update.

Fix OMPT init ordering and library typo.

ping :)

LGTM. Can I commit for you?

That would be great -- thank you!
(Hopefully, I catched everything this time :) )

ping

Incorporated feedback from @Meinersbur