Page MenuHomePhabricator

Fix build LLVM with -D LLVM_USE_INTEL_JITEVENTS:BOOL=ON on Windows
ClosedPublic

Authored by andrew.w.kaylor on Feb 5 2016, 5:04 PM.

Details

Summary

This patch updates the CMakeLists.txt file for the LLVM Intel JIT event listener to fix a problem I encountered when building on Windows. It also incorporates changes from D16883 which I was preparing for commit when I encountered the Windows problem (see https://llvm.org/bugs/show_bug.cgi?id=26433).

Rafael, I am asking you to review this because you authored the previous change which introduced the pthread and libdl dependencies (which are needed on Linux but cause failures on Windows). I've tested this on Windows and on Linux (with and without the -DBUILD_SHARED_LIBS=ON option on Linux), but I'm always finding new subtleties in the build system that I've overlooked so I figure it's worth having you take a look to make sure I didn't re-break something you fixed with the previous change.

I'm hoping these changes can be made in the 3.8 branch as well.

Diff Detail

Repository
rL LLVM

Event Timeline

andrew.w.kaylor retitled this revision from to Fix build LLVM with -D LLVM_USE_INTEL_JITEVENTS:BOOL=ON on Windows.
andrew.w.kaylor updated this object.
andrew.w.kaylor added reviewers: rafael, lhames.
andrew.w.kaylor set the repository for this revision to rL LLVM.
andrew.w.kaylor added a subscriber: llvm-commits.
rafael edited edge metadata.

This is fine by me, but you should probably get a review by someone familiar with the current CMake setup. Chris?

lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
18 ↗(On Diff #47065)

Why do you need this?

lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
18 ↗(On Diff #47065)

That's a good question. There is no reason that IntelJITEvents should need this, but there is an include of attributes.inc a few levels deep in a file the gets included from IntelJITEventListener.cpp. Something has changed in trunk in the past few days to make the build failure go away, but I haven't been able to track down what it was. The build failure still exists in the 3.8 branch. It looks like this:

Building CXX object lib/ExecutionEngine/IntelJITEvents/CMakeFiles/LLVMIntelJITEvents.dir/IntelJITEventListener.cpp.o
In file included from /users/akaylor/llvm38/llvm/include/llvm/IR/CallSite.h:31: ,
                 from /users/akaylor/llvm38/llvm/include/llvm/Analysis/AliasAnalysis.h:42,
                 from /users/akaylor/llvm38/llvm/include/llvm/CodeGen/MachineInstr.h:26,
                 from /users/akaylor/llvm38/llvm/include/llvm/CodeGen/MachineBasicBlock.h:18,
                 from /users/akaylor/llvm38/llvm/include/llvm/CodeGen/MachineFunction.h:22,
                 from /users/akaylor/llvm38/llvm/lib/ExecutionEngine/IntelJITEvents/IntelJITEventListener.cpp:18:
/users/akaylor/llvm38/llvm/include/llvm/IR/Attributes.h:69:38: fatal error: llvm/IR/Attributes.inc: No such file or directory
     #include "llvm/IR/Attributes.inc"
                                      ^
compilation terminated.

It may be that having the dependency here isn't the best way to fix this.

beanz added inline comments.Feb 10 2016, 10:45 PM
lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
3 ↗(On Diff #47065)

You don't need to set the variable to empty before using it. Evaluating a variable that has never been set is equivalent to evaluating one that was set to nothing.

18 ↗(On Diff #47065)

The correct way to handle this is probably to depend on CodeGen rather than intrinsics_gen. That dependency can be expressed either in CMake or in the LLVMBuild file. Doing that sets up the transitive dependency that a tool linking IntelJITEvents also needs CodeGen, Analysis and Core.

andrew.w.kaylor edited edge metadata.

Updated files based on review feedback

lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
17 ↗(On Diff #47712)

I'm not 100% sure that the change I made matches what you are suggesting, but it appears to fix the problem in the 3.8 branch.

beanz accepted this revision.Feb 11 2016, 2:03 PM
beanz edited edge metadata.

One minor comment, but LGTM.

lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
17 ↗(On Diff #47712)

Just a nitpick. When linking LLVM components instead of calling add_dependencies you can set LLVM_LINK_COMPONENTS before calling add_llvm_library.

Adding something like this to the top of the file is the more normal way to handle this:

set(LLVM_LINK_COMPONENTS Support)
This revision is now accepted and ready to land.Feb 11 2016, 2:03 PM
lib/ExecutionEngine/IntelJITEvents/CMakeLists.txt
17 ↗(On Diff #47712)

That doesn't appear to fix the build problem.

I tried

set(LLLVM_LINK_COMPONENTS CodeGen)

and

set(LLVM_LINK_COMPONENTS Support CodeGen)

Could this be an issue because I am declaring a library and not a tool?

All the similar files I've looked at seem to be using some form of adding a dependency directly on intrinsics_gen. Many of them seem to have come from r239796 (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150615/281739.html).

beanz added a comment.Feb 16 2016, 3:51 PM

Sorry about that. Yea, go ahead and commit as is.

Thanks,
-Chris

This revision was automatically updated to reflect the committed changes.