This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add dependency on GlobalISel for unit tests to fix shared libs build
ClosedPublic

Authored by nemanjai on Oct 16 2019, 12:19 PM.

Details

Diff Detail

Event Timeline

nemanjai created this revision.Oct 16 2019, 12:19 PM
rovka added a comment.Oct 17 2019, 1:54 AM

Could you be a little more specific? I just ran a build with BUILD_SHARED_LIBS=ON and it seemed to work.

unittests/CodeGen/CMakeLists.txt
12 ↗(On Diff #225286)

Why is this necessary? There's a subdirectory for GlobalISel, if some unit test in here is pulling in GlobalISel it's worth investigating why.

unittests/Target/ARM/CMakeLists.txt
15 ↗(On Diff #225286)

Nit; These lists are sorted alphabetically.

nemanjai marked 2 inline comments as done.Oct 17 2019, 4:06 AM
nemanjai added inline comments.
unittests/CodeGen/CMakeLists.txt
12 ↗(On Diff #225286)

Sorry, I probably should have added that to the description. It is missing vtables for two classes.

unittests/Target/ARM/CMakeFiles/ARMTests.dir/MachineInstrTest.cpp.o:(.toc+0x0): undefined reference to `vtable for llvm::RegisterBankInfo'
unittests/Target/ARM/CMakeFiles/ARMTests.dir/MachineInstrTest.cpp.o:(.toc+0x8): undefined reference to `vtable for llvm::LegalizerInfo'

Build info:
GCC build compiler: 7.3.1
CMake vars: -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON
OS: Ubuntu 16.04.6

unittests/Target/ARM/CMakeLists.txt
15 ↗(On Diff #225286)

Oops, sorry about that. I'll reorder them.

nemanjai updated this revision to Diff 225490.Oct 17 2019, 11:58 AM

Added the library in its correct slot alphabetically.

nemanjai marked an inline comment as done.Oct 17 2019, 11:58 AM

The references are coming from ARMSubtarget which is instantiated in MachineInstrTest.cpp. These have in the ARMSubtarget.h header file:

/// GlobalISel related APIs.
std::unique_ptr<CallLowering> CallLoweringInfo;
std::unique_ptr<InstructionSelector> InstSelector;
std::unique_ptr<LegalizerInfo> Legalizer;
std::unique_ptr<RegisterBankInfo> RegBankInfo;

There are code-generation differences in GCC and Clang. GCC generates the destructors for RegBankInfo and Legalizer in Comdat groups and calls them from the destructor of ArmSubTargetInfo, whereas LLVM does not. I don't intuitively know why there is this difference in code-generation.

In the static build libLLVMGlobalISel.a is included in the link line whereas it isn't for the shared build. I think that unless there is a good reason for GCC not to output the destructors then it is worth adding GlobalIsel.

nemanjai marked an inline comment as done.Oct 18 2019, 6:31 AM

The references are coming from ARMSubtarget which is instantiated in MachineInstrTest.cpp. These have in the ARMSubtarget.h header file:

/// GlobalISel related APIs.
std::unique_ptr<CallLowering> CallLoweringInfo;
std::unique_ptr<InstructionSelector> InstSelector;
std::unique_ptr<LegalizerInfo> Legalizer;
std::unique_ptr<RegisterBankInfo> RegBankInfo;

There are code-generation differences in GCC and Clang. GCC generates the destructors for RegBankInfo and Legalizer in Comdat groups and calls them from the destructor of ArmSubTargetInfo, whereas LLVM does not. I don't intuitively know why there is this difference in code-generation.

In the static build libLLVMGlobalISel.a is included in the link line whereas it isn't for the shared build. I think that unless there is a good reason for GCC not to output the destructors then it is worth adding GlobalIsel.

We don't really have control over how the GCC build compiler works, so I really think we should go ahead with this patch. This is particularly important since the PPC bot has also been failing for 3 days or so with the same error (caused by r374887):
http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/10972

I initially didn't realize that this is causing build bot breaks, so I plan to commit this patch today to get the bot back to green.

unittests/CodeGen/CMakeLists.txt
8 ↗(On Diff #225490)

This one is actually not needed. Just the other one. So on the commit, I'll commit just the change in unittests/Target/ARM.

rovka accepted this revision.Oct 18 2019, 9:05 AM

Thanks, LGTM then!

This revision is now accepted and ready to land.Oct 18 2019, 9:05 AM
This revision was automatically updated to reflect the committed changes.