Page MenuHomePhabricator

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

Authored by mehdi_amini on Fri, Nov 13, 5:29 PM.

Details

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

Diff Detail

Event Timeline

mehdi_amini created this revision.Fri, Nov 13, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 13, 5:29 PM
mehdi_amini requested review of this revision.Fri, Nov 13, 5:29 PM

@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.

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 accepted this revision.Fri, Nov 13, 5:35 PM

@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.

This revision is now accepted and ready to land.Fri, Nov 13, 5:35 PM

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?

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?

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?

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.

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.

This revision was landed with ongoing or failed builds.Fri, Nov 13, 6:19 PM
This revision was automatically updated to reflect the committed changes.

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.

We should dig in the OpenMP side as well.

What do you mean by that?

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 added inline comments.Fri, Nov 13, 8:16 PM
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.

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.

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.

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.