This is an archive of the discontinued LLVM Phabricator instance.

[libc] Place headers in the right include directory
ClosedPublic

Authored by phosek on Jun 9 2023, 3:44 PM.

Details

Summary

When LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is enabled, place headers
in include/<target> directory, otherwise use include/.

Diff Detail

Event Timeline

phosek created this revision.Jun 9 2023, 3:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2023, 3:44 PM
phosek requested review of this revision.Jun 9 2023, 3:44 PM
phosek added inline comments.Jun 9 2023, 3:47 PM
libc/CMakeLists.txt
29

@sivachandra When LIBC_TARGET_ARCHITECTURE_IS_GPU, would it make sense to use gpu-none-llvm instead of LLVM_DEFAULT_TARGET_TRIPLE? It seems like we're effectively treating gpu-none-llvm as a target triple but I wanted to confirm first before making the change.

32–33

@sivachandra Would it make sense to use ${CMAKE_CURRENT_BINARY_DIR}/lib in this case for consistency?

phosek marked 2 inline comments as not done.Jun 9 2023, 3:47 PM
sivachandra accepted this revision.Jun 9 2023, 11:10 PM
sivachandra added a subscriber: jhuber6.
sivachandra added inline comments.
libc/CMakeLists.txt
29

If building for the GPU, I think we want the target triple to be fixed to gpu-none-llvm and LLVM_DEFAULT_TARGET_TRIPLE does not matter. I will let @jhuber6 to confirm.

32–33

Agreed.

libc/include/CMakeLists.txt
534

What is this target_include_directories doing? I can see why it could be ${LIBC_INCLUDE_DIR} but I do not understand why ${LLVM_BINARY_DIR}/include.

This revision is now accepted and ready to land.Jun 9 2023, 11:10 PM
jhuber6 added inline comments.Jun 10 2023, 4:32 AM
libc/CMakeLists.txt
29

Yes, that was a decision to separate these generates GPU headers from the ones that would be used by the system. Since we don't have a single triple we just made one up.

phosek updated this revision to Diff 530725.Jun 12 2023, 5:14 PM
phosek marked 4 inline comments as done.
phosek updated this revision to Diff 533327.Jun 21 2023, 10:39 AM
This revision was automatically updated to reflect the committed changes.
jhuber6 added a comment.EditedJun 23 2023, 6:11 AM

This broke the GPU headers in many ways, I'll need to look into it. It's now installing the headers somewhere completely outside of the CMAKE_PREFIX_PATH because the relative paths are wrong.

jhuber6 added inline comments.Jun 23 2023, 6:22 AM
libc/CMakeLists.txt
22–28

This does not work, this macro isn't defined until line 107 below.

libc/cmake/modules/LLVMLibCHeaderRules.cmake
23 ↗(On Diff #533476)

The old logic used to create for example this file when doing a runtimes build.

./runtimes/runtimes-bins/libc/include/llvm-libc-types/rpc_opcodes_t.h

Now I get

./include/gpu-none-llvm/llvm-libc-types/rpc_opcodes_t.h

Which is wrong because we should not be building these outside of the runtimes directory when doing a runtimes build, and because of the discrepancy it causes the relative path to be completely wrong. Did you test this with a runtimes build of libc?

phosek added inline comments.Jun 23 2023, 10:43 AM
libc/cmake/modules/LLVMLibCHeaderRules.cmake
23 ↗(On Diff #533476)

This is intentional and matches what we do for other runtimes like libc++. Our goal has always been for the layout of the build directory to match the layout of the install directory so you can run the compiler without having to do the install step first. Clang driver looks for headers in <path to clang>/../include/.... If this is undesirable for the GPU target, we can adjust the logic, but we want to keep this for other targets.