Page MenuHomePhabricator

[openmp] Disable archer if LIBOMP_OMPT_SUPPORT is off
ClosedPublic

Authored by mgorny on Jan 22 2020, 9:59 PM.

Diff Detail

Event Timeline

mgorny created this revision.Jan 22 2020, 9:59 PM

Seems reasonable, @protze.joachim agreed?

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/.

If by 'guaranteed' you mean in the current code, then I've tested this patch with it being both true and false.

Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

I don't see a problem moving it. However, I'm not sure if rename is really desired. After all, AFAIU it's a characteristic of the library.

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/.

If by 'guaranteed' you mean in the current code, then I've tested this patch with it being both true and false.

Ok, as long as it works for the release (that's probably the motivation?) I'm fine with this.

Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

I don't see a problem moving it. However, I'm not sure if rename is really desired. After all, AFAIU it's a characteristic of the library.

Currently yes, but OMPT callbacks might also be added to libomptarget. We'll need to move then at the latest, but I agree that it might be out-of-scope for this patch (I was just thinking for solutions if LIBOMP_OMPT_SUPPORT did not work here).

Yes, indeed the goal is to unbreak 10.x. I'd also agree to making bigger changes in 11.x separately.

@Hahnfeld can you accept, assuming you are happy with this as a fix for now?

This revision is now accepted and ready to land.Jan 23 2020, 9:21 AM

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

From my understanding of cmake scoping, this variable is visible because it's a cached option?

From my understanding of cmake scoping, this variable is visible because it's a cached option?

Yes. It is also important that runtime directory is added before tools, so the default is set before it is checked.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2020, 10:30 AM