Switch libLLVMFrontendOpenACC to be a regular CMake library and not a "component"

[mehdi_amini]

Summary

This library is only used in Flang at the moment and not build and
released with LLVM. Having it as a component is breaking llvm-config:

$ bin/llvm-config --shared-mode
llvm-config: error: component libraries and shared library

llvm-config: error: missing: [...]/lib/libLLVMFrontendOpenACC.a

[mehdi_amini]

@clementval : it isn't clear why this is in llvm/lib/Frontend/? Should this be a top-level frontend/lib kind of structure? There isn't any user of this inside LLVM right now other than flang and it isn't clear to me how it connects to LLVM itself.

[mehdi_amini]

It seems like it is following what was done for OpenMP, however OpenMP is exposing LLVM IR Builder which connects it directly to LLVM. The directive part of the OpenMP library isn't tested either (uhhhh) in LLVM, it just does not expose the issue because it is bundles in the same library as the IR builders which are tested.

[clementval]

In D91470#2395385, @mehdi_amini wrote:

@clementval : it isn't clear why this is in llvm/lib/Frontend/? Should this be a top-level frontend/lib kind of structure? There isn't any user of this inside LLVM right now other than flang and it isn't clear to me how it connects to LLVM itself.

You are right, at the moment it is not connected directly to LLVM itself. It was done like this a little bit blindly follow what was done for for the libLLVMFrontendOpenMP.

[mehdi_amini]

In D91470#2395391, @clementval wrote:

You are right, at the moment it is not connected directly to LLVM itself. It was done like this a little bit blindly follow what was done for for the libLLVMFrontendOpenMP.

Is the code in ACC.cpp the kind of thing that could be unit-tested?

[clementval]

In D91470#2395395, @mehdi_amini wrote:

In D91470#2395391, @clementval wrote:

You are right, at the moment it is not connected directly to LLVM itself. It was done like this a little bit blindly follow what was done for for the libLLVMFrontendOpenMP.

Is the code in ACC.cpp the kind of thing that could be unit-tested?

Yeah some function could have some unit tests. Would it make sense to send a patch with some tests?

[mehdi_amini]

I think it is very unusual to have a library in LLVM that isn't used in LLVM and so does not come with any testing, so yes it'd be better to do so.
I'm not actually sure if llvm-config just relies implicitly on this?

I can land this for now, and you can revert this when adding tests?

[clementval]

In D91470#2395409, @mehdi_amini wrote:

I think it is very unusual to have a library in LLVM that isn't used in LLVM and so does not come with any testing, so yes it'd be better to do so.
I'm not actually sure if llvm-config just relies implicitly on this?

Yeah it makes sense to have tests directly in llvm. Sorry for that I blindly followed the OpenMP stuffs and I should have think twice about it.

I can land this for now, and you can revert this when adding tests?

Sure go ahead if it fixes the current problem. I can revert together with the tests.

[mehdi_amini]

In D91470#2395424, @clementval wrote:

Yeah it makes sense to have tests directly in llvm. Sorry for that I blindly followed the OpenMP stuffs and I should have think twice about it.

No worry: the OpenMP side is setup without tests you couldn't anticipate... We should dig in the OpenMP side as well.

[Meinersbur]

Like IRBuilder and OpenMPIRBuilder, I think the OpenACC stuff is meant to be used by frontends outside of LLVM (in the sense of /llvm/*) and thus should be exposed as a component and llvm-config, but tested independently of those outside users. OpenACC is standardized for C/C++ as well.

In D91470#2395433, @mehdi_amini wrote:

We should dig in the OpenMP side as well.

What do you mean by that?

[mehdi_amini]

In D91470#2395433, @mehdi_amini wrote:

We should dig in the OpenMP side as well.

What do you mean by that?

I mean that commenting out OMP.cpp here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Frontend/OpenMP/CMakeLists.txt#L6 and running ninja check-llvm just works.

[mehdi_amini]

llvm/lib/Frontend/OpenACC/CMakeLists.txt

17

It has to be a component right now because of the dependency on LLVMSupport which breaks the build when linking against libLLVM.so.

[vitalybuka]

Some bots are still broken
http://lab.llvm.org:8011/#/builders/85/builds/811
http://lab.llvm.org:8011/#/builders/74/builds/857
http://lab.llvm.org:8011/#/builders/99/builds/670

[mehdi_amini]

In D91470#2395621, @vitalybuka wrote:

Some bots are still broken
http://lab.llvm.org:8011/#/builders/85/builds/811
http://lab.llvm.org:8011/#/builders/74/builds/857
http://lab.llvm.org:8011/#/builders/99/builds/670

I reverted this patch, what remains is the original failure after the patch from @serge-sans-paille . I didn't know we had bot broken though, I pushed d34add0ba1c2 just now which should fix the bots.

[Meinersbur]

In D91470#2395495, @mehdi_amini wrote:

In D91470#2395433, @mehdi_amini wrote:

We should dig in the OpenMP side as well.

What do you mean by that?

I mean that commenting out OMP.cpp here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Frontend/OpenMP/CMakeLists.txt#L6 and running ninja check-llvm just works.

I see. We should add some unittests for the functions in that file.

[clementval]

In D91470#2398240, @Meinersbur wrote:

I see. We should add some unittests for the functions in that file.

I'm gonna submit a patch today with unit tests for the OpenACC part. Will check with the OpenMP folks to add some unit tests as well.

llvm/lib/Frontend/OpenACC/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend/OpenACC/ACC.td)
 tablegen(LLVM ACC.cpp --gen-directive-impl)
 add_public_tablegen_target(acc_cpp)
 
-add_llvm_component_library(LLVMFrontendOpenACC
+add_llvm_library(LLVMFrontendOpenACC
   ACC.cpp # Generated by tablegen above
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend/OpenACC
 
   DEPENDS
   acc_gen
   acc_cpp
 )
 
 target_link_libraries(LLVMFrontendOpenACC LLVMSupport)