This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] Build runtime with OMPT support by default
ClosedPublic

Authored by protze.joachim on Dec 21 2017, 11:33 AM.

Details

Summary

As discussed on openmp-dev mailing list.

This patch enables OMPT by default if version 50 or later is built and the config says, that OMPT will be supported.

Diff Detail

Repository
rL LLVM

Event Timeline

Hahnfeld requested changes to this revision.Dec 22 2017, 2:24 AM
Hahnfeld edited reviewers, added: pawosm01, hfinkel; removed: pawelo.

I don't think we should enable this unconditionally, only for the architectures where we tested and are sure that the tests will pass: x86, x86_64, ppc64, aarch64 on Linux, macOS if we want to. With the current change we will get into trouble at least on MIPS(64).

runtime/CMakeLists.txt
305–309 ↗(On Diff #127921)

I think it would be better to only have one definition of cache variables:

set(OMPT_DEFAULT FALSE)
if (...)
  set(OMPT_DEFAULT TRUE)
endif()
set(LIBOMP_OMPT_SUPPORT TRUE CACHE BOOL
    "Enable support for OMPT?")
This revision now requires changes to proceed.Dec 22 2017, 2:24 AM

I added the requirement for hardware architecture support to LIBOMP_HAVE_OMPT_SUPPORT.
This way one would also get an error when manually turing LIBOMP_OMPT_SUPPORT=on on an unsupported architecture.

Also implemented Jonas' suggestion.

protze.joachim marked an inline comment as done.Dec 23 2017, 1:58 AM
Hahnfeld added inline comments.Dec 23 2017, 2:48 AM
runtime/cmake/config-ix.cmake
242 ↗(On Diff #128073)

I think we haven't adjusted the tests for ARM 32 bit yet? @pawosm01

@Hahnfeld gosh, you're right and I don't work with 32-bit systems nowadays so I need to track down one of those still kept online. Being far from the office isn't helping, so it won't happen immediately.

Removed 32 bit ARM from the list of supported architectures until it is tested.

Looks good so far. Can you document the change of the default value in the (just committed) README.rst?

Updated the README

Hahnfeld accepted this revision.Jan 2 2018, 3:39 AM

LG. I'd feel better with an explicit acknowledgment from Intel though...

(We should probably add a few sentences to Clang's release notes so that users know the reason should they see a performance degradation. I can handle that after this patch landed.)

README.rst
195–196 ↗(On Diff #128403)

Should we mention which platforms are supported? Like This option is ON by default for x86, x86_64, AArch64, and PPC64 on Linux, Windows, and mac OS?

This revision is now accepted and ready to land.Jan 2 2018, 3:39 AM

Updated the README

This revision was automatically updated to reflect the committed changes.