[openmp][testing] disable check targets if not building with clang

[makslevental]

Currently you can't build the openmp project (i.e., -DLLVM_ENABLE_PROJECTS=mlir;openmp) without also building clang. This fixes that and enables building the various openmp libs (such as libomp) by projects that have no need of clang.

[jpienaar]

To check: so the only reason this was true was due to testing?

[makslevental]

To check: so the only reason this was true was due to testing?

Don't hold me to that. Yes with just this patch you can configure and build, but in addition I have

-DLIBOMP_OMPD_GDB_SUPPORT=OFF
-DLIBOMP_USE_QUAD_PRECISION=False
-DOPENMP_ENABLE_LIBOMPTARGET=OFF

but I don't remember why.

But irrespective of whether this patch is sufficient, it's definitely necessary.

[shiltian]

This doesn't look right. Basically, libomp can be built with any compatible compiler, while libomptarget requires in-tree clang. We can test libomp w/o need of clang but indeed we need clang to test libomptarget. This changes can affect both.

[makslevental]

This doesn't look right. Basically, libomp can be built with any compatible compiler, while libomptarget requires in-tree clang. We can test libomp w/o need of clang but indeed we need clang to test libomptarget. This changes can affect both.

Can you suggest an alternative patch then? Isn't it clearly a bug that I absolutely can't build libomp without building in-tree clang even though you concede it is possible and fine?

This changes can affect both.

I don't see how this affectst actually anything at all since people wanting to build/test libomptarget (e.g., the builder bots) will indeed enable both clang and openmp projects?

[makslevental]

Suggested change

	set(ENABLE_CHECK_TARGETS FALSE)
	set(ENABLE_CHECK_TARGETS FALSE)
	set(OPENMP_ENABLE_LIBOMPTARGET OFF)

@shiltian would you accept someting like this (with an amended warning)?

[makslevental]

Suggested change

	if (NOT TARGET "clang")
	find_program(OPENMP_CLANG_EXECUTABLE "clang")
	if (NOT TARGET "clang" AND NOT OPENMP_CLANG_EXECUTABLE)

[shiltian]

If ENABLE_CHECK_TARGETS is set to FALSE, that indicates all OpenMP tests will be disabled, including both libomp tests and libomptarget tests. However, even w/o clang, chances are that libomp can still be tested by almost any compatible compiler.
I'd just disable libomptarget tests if clang doesn't exist, no matter it is a target or a program.

[makslevental]

libomptarget

the problem is the tests aren't factored in such a way that that's possible right? When you don't have a clang target available, the actual CMake error comes from the add_lit_testsuite depending on clang here:

    if (ARG_EXCLUDE_FROM_CHECK_ALL)
      add_lit_testsuite(${target}
        ${comment}
        ${ARG_UNPARSED_ARGUMENTS}
        EXCLUDE_FROM_CHECK_ALL
        DEPENDS clang FileCheck not ${ARG_DEPENDS}
        ARGS ${ARG_ARGS}
      )
    else()
      add_lit_testsuite(${target}
        ${comment}
        ${ARG_UNPARSED_ARGUMENTS}
        DEPENDS clang FileCheck not ${ARG_DEPENDS}
        ARGS ${ARG_ARGS}
      )
    endif()

So fiddling with libomptarget tests won't work.

[makslevental]

I should note that in putting together this PR I did discover that this combination of flags is a workaroud

-DOPENMP_STANDALONE_BUILD=ON
-DOPENMP_ENABLE_LIBOMPTARGET=OFF

but I don't like it because this ends up building libclang for some reason (which I would of course prefer not do).

[shiltian]

I see your point. The current implementation is wrong and quite unfortunate. The correct fix would be first to remove the unconditional clang as dependence, and then add it accordingly.

[makslevental]

Actually I'm wrong - it's not libclang but definitely something that weighs in at ~300 new build targets that I didn't have with this patch. Okay anyway I'm closing because while I think this is broken, I can't spend time to fix right now and the workaround is sufficient. Thanks @shiltian for the discussion.

[makslevental]

@shiltian is there a reason no exported target is created during standalone build?

if (NOT OPENMP_STANDALONE_BUILD)
  get_target_export_arg(omp LLVM export_to_llvmexports)
  set_property(GLOBAL APPEND PROPERTY LLVM_EXPORTS omp)
endif()

I tried removing the NOT just now and it seems to work fine?

[shiltian]

@shiltian is there a way no exported target is created during standalone build?

if (NOT OPENMP_STANDALONE_BUILD)
  get_target_export_arg(omp LLVM export_to_llvmexports)
  set_property(GLOBAL APPEND PROPERTY LLVM_EXPORTS omp)
endif()

I tried removing the NOT just now and it seems to work fine?

AFAIK, the issue only exists on macOS. I didn't see that on either Windows or Linux. In standalone build, we don't need to export omp to LLVM.

[makslevental]

Sorry I meant to ask is there reason it's not exported.

AFAIK, the issue only exists on macOS. I didn't see that on either Windows or Linux.

Which issue do you mean?

In standalone build, we don't need to export omp to LLVM.

You don't need but I would like it - as I mentioned, it's useful for other projects that might want to use libomp (such as MLIR).

[shiltian]

Which issue do you mean?

https://reviews.llvm.org/D149617

In standalone build, we don't need to export omp to LLVM.

You don't need but I would like it - as I mentioned, it's useful for other projects that might want to use libomp (such as MLIR).

That's two separate things. This "export" is to export to LLVM. When OpenMP is built standalone, there is no "LLVM" to export to, which I think will cause CMake error. When OpenMP is not built standalone, which means it is built in-tree, there will be a LLVM out there to accept targets.

In your case, you worked around some build issues by forcing OPENMP_STANDALONE_BUILD to TRUE while in fact it is not.