[libclc] Fix a couple of issues preventing in-tree builds #87505

frasercrmck · 2024-04-03T15:20:37Z

libclc is mentioned in the list of LLVM_ENABLE_PROJECTS but it isn't actually possible to build it in-tree for various reasons. Users currently have to build it via LLVM_ENABLE_EXTERNAL_PROJECTS, which isn't very well documented.

We can't properly build in-tree because the current system needs to "see" clang and other tools at CMake configuration time. The general idea is that we could fix this in the future by moving the compilation and linking of bitcode libraries to custom commands, which would remove the dependency on CMake configuration and would allow us to build libclc after clang and other tools are built in-tree. Since that's a bigger change, it is being left for later.

Note that with this commit it's still not possible to properly build in-tree - this commit just fixes a few little things that are in the way. We are now able to build in-tree in the sense that it can be built as a regular LLVM sub-project, but the tools it uses to compile the libraries are still picked up from a pre-existing installation of LLVM, and not from tools built during the same build as libclc.

The things fixed by this commit include:

Its use of CMAKE_SOURCE_DIR (i.e., assuming it was the top-level project)
- These have been converted to PROJECT_SOURCE_DIR - should have no consequences for out-of-tree builds.
Its prepare_builtins tool insisting on linking against the dynamic LLVM.so.
- This has been turned from an "llvm executable" into an "llvm utility" which links against the static libraries.
- It was also missing a link component for the IRReader library.
Assuming an output path for its builtin libraries (dependent on the working directory)
- This has been changed to query CMake for the library target's output file.
The spirv-mesa3d and spirv64-mesa3d targets were enabled by default (or when asking to build 'all' libclc targets), when they require llvm-spirv as an external dependency.
- They are now only built when the user explicitly asks for them, or when llvm-spirv is available and the user asks for 'all'.

libclc is mentioned in the list of LLVM_ENABLE_PROJECTS but it isn't actually possible to build it in-tree for various reasons. Users currently have to build it via LLVM_ENABLE_EXTERNAL_PROJECTS, which isn't very well documented. We can't properly build in-tree because the current system needs to "see" clang and other tools at CMake configuration time. The general idea is that we could fix this in the future by moving the compilation and linking of bitcode libraries to custom commands, which would remove the dependency on CMake configuration and would allow us to build libclc after clang and other tools are built in-tree. Since that's a bigger change, it is being left for later. Note that with this commit it's *still* not possible to properly build in-tree - this commit just fixes a few little things that are in the way. We are now able to build in-tree in the sense that it can be built as a regular LLVM sub-project, but the tools it uses to compile the libraries are still picked up from a pre-existing installation of LLVM, and not from tools built during the same build as libclc. The things fixed by this commit include: * Its use of CMAKE_SOURCE_DIR (i.e., assuming it was the top-level project) * These have been converted to PROJECT_SOURCE_DIR - should have no consequences for out-of-tree builds. * Its prepare_builtins tool insisting on linking against the dynamic LLVM.so. * This has been turned from an "llvm executable" into an "llvm utility" which links against the static libraries. * It was also missing a link component for the IRReader library. * Assuming an output path for its builtin libraries (dependent on the working directory) * This has been changed to query CMake for the library target's output file. * The spirv-mesa3d and spirv64-mesa3d targets were enabled by default (or when asking to build 'all' libclc targets), when they require llvm-spirv as an external dependency. * They are now only built when the user explicitly asks for them, or when llvm-spirv is available and the user asks for 'all'.

frasercrmck · 2024-04-03T15:24:51Z

CC @rjodinchr - I can't seem to request a review from you.

I wasn't sure whether or not to change llvm/tools/CMakeLists.txt to add libclc if requested. As mentioned in the description, it would start building libclc "in-tree" but would be using tools from the system LLVM installation, or fail if they're not present, which seemst dangerous to do at this stage.

I still don't really get if we're building libclc as part of pull requests or not.

rjodinchr

I have been trying to build libclc as an external project here, but I cannot manage to figure out the issue I have on windows.
Looking forward to being able to build libclc in-tree!

mgorny · 2024-04-06T09:11:26Z

This change broke standalone build against LLVM dylib — it now insists on linking to non-existing static libraries:

FAILED: prepare_builtins 
: && /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -march=znver2 --param=l1-cache-size=32 --param=l1-cache-line-size=64 -O2 -pipe -frecord-gcc-switches -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0     -Wl,-rpath-link,  -Wl,--gc-sections CMakeFiles/prepare_builtins.dir/utils/prepare-builtins.cpp.o -o prepare_builtins  -Wl,-rpath,"\$ORIGIN/../lib64:/usr/lib/llvm/17/lib64"  -lLLVMBitReader  -lLLVMBitWriter  -lLLVMCore  -lLLVMIRReader  /usr/lib/llvm/17/lib64/libLLVMSupport.a  -lrt  -ldl  -lm  /usr/lib64/libz3.so  /usr/lib64/libz.so  /usr/lib64/libzstd.so  /usr/lib64/libtinfo.so  /usr/lib/llvm/17/lib64/libLLVMDemangle.a && :
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMBitReader: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMBitWriter: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMCore: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMIRReader: No such file or directory
collect2: error: ld returned 1 exit status

Given that it apparently doesn't fix in-tree builds fully, please revert and let's figure out how to do it properly without breaking its primary use case.

frasercrmck · 2024-04-08T10:10:47Z

This change broke standalone build against LLVM dylib — it now insists on linking to non-existing static libraries:

FAILED: prepare_builtins 
: && /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -march=znver2 --param=l1-cache-size=32 --param=l1-cache-line-size=64 -O2 -pipe -frecord-gcc-switches -Wl,-O1 -Wl,--as-needed -Wl,--defsym=__gentoo_check_ldflags__=0     -Wl,-rpath-link,  -Wl,--gc-sections CMakeFiles/prepare_builtins.dir/utils/prepare-builtins.cpp.o -o prepare_builtins  -Wl,-rpath,"\$ORIGIN/../lib64:/usr/lib/llvm/17/lib64"  -lLLVMBitReader  -lLLVMBitWriter  -lLLVMCore  -lLLVMIRReader  /usr/lib/llvm/17/lib64/libLLVMSupport.a  -lrt  -ldl  -lm  /usr/lib64/libz3.so  /usr/lib64/libz.so  /usr/lib64/libzstd.so  /usr/lib64/libtinfo.so  /usr/lib/llvm/17/lib64/libLLVMDemangle.a && :
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMBitReader: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMBitWriter: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMCore: No such file or directory
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lLLVMIRReader: No such file or directory
collect2: error: ld returned 1 exit status

Given that it apparently doesn't fix in-tree builds fully, please revert and let's figure out how to do it properly without breaking its primary use case.

Ach, sorry about that, thanks for catching that. It's tricky without CI to catch this kind of thing, though that was particularly silly of me. I actually struggled to build LLVM locally with the kind of configuration that only has dynamic libraries available to reproduce.

That said, I've pushed 8461d90 which should just re-instate the old behaviour for out-of-tree builds. If that doesn't fix the issue then I'll revert properly - please let me know, thanks.

mgorny · 2024-04-08T13:58:37Z

Thanks. This seems to fix that problem. However, now I'm seeing missing dep in build ordering:

[1793/1922] cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.opt.spirv-mesa3d-.bc
FAILED: spirv-mesa3d-.spv /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build/spirv-mesa3d-.spv 
cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.opt.spirv-mesa3d-.bc
No such file or directory
[1794/1922] /usr/lib/llvm/17/bin/clang -DCLC_SPIRV64 -D__CLC_INTERNAL -I/usr/lib/llvm/17/include -I/tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc/generic/include -cl-no-stdinc -target spir64-- -fno-builtin -nostdlib -O0 -finline-hint-functions -I /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc/generic/lib/shared -MD -MT CMakeFiles/builtins.link.spirv64-mesa3d-.dir/generic/lib/shared/vload.bc -MF CMakeFiles/builtins.link.spirv64-mesa3d-.dir/generic/lib/shared/vload.bc.d -o CMakeFiles/builtins.link.spirv64-mesa3d-.dir/generic/lib/shared/vload.bc -c /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc/generic/lib/shared/vload.cl -emit-llvm
[1795/1922] cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/opt -o builtins.opt.spirv-mesa3d-.bc /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build/builtins.link.spirv-mesa3d-.bc

Note that builtins.link.spirv-mesa3d-.bc is generated in step 1795, while it's used in 1793. Not sure if it's related, but I've never seen that one before.

mgorny · 2024-04-08T14:04:14Z

      set( spv_suffix ${arch_suffix}.spv )
      add_custom_command( OUTPUT "${spv_suffix}"
        COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
        DEPENDS ${builtins_link_lib_tgt} )

I think that ought to be DEPENDS ${builtins_opt_lib_tgt}. Do you need me to send a PR for that?

frasercrmck · 2024-04-08T14:23:15Z

      set( spv_suffix ${arch_suffix}.spv )
      add_custom_command( OUTPUT "${spv_suffix}"
        COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
        DEPENDS ${builtins_link_lib_tgt} )

I think that ought to be DEPENDS ${builtins_opt_lib_tgt}. Do you need me to send a PR for that?

Hmm yes you might be right. Just to confirm, does that fix the problem for you?

I can't be sure but it seems like that was a problem before 61efea7 when I last touched it, as least from a simple reading of the CMake.

rjodinchr · 2024-04-08T14:28:17Z

      set( spv_suffix ${arch_suffix}.spv )
      add_custom_command( OUTPUT "${spv_suffix}"
        COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
        DEPENDS ${builtins_link_lib_tgt} )
I think that ought to be DEPENDS ${builtins_opt_lib_tgt}. Do you need me to send a PR for that?
Hmm yes you might be right. Just to confirm, does that fix the problem for you?

I can't be sure but it seems like that was a problem before 61efea7 when I last touched it, as least from a simple reading of the CMake.

I think the issue is not coming from the DEPENDS but from the source.

- COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
+ COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_link_lib_tgt}

frasercrmck · 2024-04-08T14:29:40Z

      set( spv_suffix ${arch_suffix}.spv )
      add_custom_command( OUTPUT "${spv_suffix}"
        COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
        DEPENDS ${builtins_link_lib_tgt} )
I think that ought to be DEPENDS ${builtins_opt_lib_tgt}. Do you need me to send a PR for that?
Hmm yes you might be right. Just to confirm, does that fix the problem for you?
I can't be sure but it seems like that was a problem before 61efea7 when I last touched it, as least from a simple reading of the CMake.
I think the issue is not coming from the DEPENDS but from the source.
- COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
+ COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_link_lib_tgt}

Yes, that's it.

frasercrmck · 2024-04-08T14:48:53Z

      set( spv_suffix ${arch_suffix}.spv )
      add_custom_command( OUTPUT "${spv_suffix}"
        COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
        DEPENDS ${builtins_link_lib_tgt} )
I think that ought to be DEPENDS ${builtins_opt_lib_tgt}. Do you need me to send a PR for that?
Hmm yes you might be right. Just to confirm, does that fix the problem for you?
I can't be sure but it seems like that was a problem before 61efea7 when I last touched it, as least from a simple reading of the CMake.
I think the issue is not coming from the DEPENDS but from the source.
- COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_opt_lib_tgt}
+ COMMAND ${LLVM_SPIRV} ${spvflags} -o "${spv_suffix}" ${builtins_link_lib_tgt}
Yes, that's it.

Should be fixed by f46f646

mgorny · 2024-04-08T15:44:25Z

Thanks. Unfortunately, I'm still getting a build failure:

[1792/1922] cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.link.spirv-mesa3d-
FAILED: spirv-mesa3d-.spv /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build/spirv-mesa3d-.spv 
cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.link.spirv-mesa3d-
No such file or directory

It seems to be missing .bc suffix, I guess? Looking at the earlier mention, shouldn't it be $<TARGET_FILE:${builtins_link_lib_tgt}>?

frasercrmck · 2024-04-08T15:54:53Z

Thanks. Unfortunately, I'm still getting a build failure:

[1792/1922] cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.link.spirv-mesa3d-
FAILED: spirv-mesa3d-.spv /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build/spirv-mesa3d-.spv 
cd /tmp/portage/dev-libs/libclc-19.0.0.9999/work/libclc_build && /usr/lib/llvm/17/bin/llvm-spirv --spirv-max-version=1.1 -o spirv-mesa3d-.spv builtins.link.spirv-mesa3d-
No such file or directory

It seems to be missing .bc suffix, I guess? Looking at the earlier mention, shouldn't it be $<TARGET_FILE:${builtins_link_lib_tgt}>?

Sorry, it should be properly fixed now - I was rushing things and that just made it worse. I hadn't noticed that the build I was testing hadn't found llvm-spirv and so those targets were missing. I've now built it locally successfully.

mgorny · 2024-04-08T16:03:22Z

Yeah, it built this time for me too. Thanks, again!

frasercrmck requested review from arsenm and tstellar April 3, 2024 15:20

arsenm approved these changes Apr 3, 2024

View reviewed changes

rjodinchr approved these changes Apr 3, 2024

View reviewed changes

frasercrmck merged commit 61efea7 into llvm:main Apr 4, 2024
5 checks passed

frasercrmck deleted the fix-in-tree-libclc branch April 4, 2024 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libclc] Fix a couple of issues preventing in-tree builds #87505

[libclc] Fix a couple of issues preventing in-tree builds #87505

frasercrmck commented Apr 3, 2024

frasercrmck commented Apr 3, 2024

rjodinchr left a comment

mgorny commented Apr 6, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024

mgorny commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

rjodinchr commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024

[libclc] Fix a couple of issues preventing in-tree builds #87505

[libclc] Fix a couple of issues preventing in-tree builds #87505

Conversation

frasercrmck commented Apr 3, 2024

frasercrmck commented Apr 3, 2024

rjodinchr left a comment

Choose a reason for hiding this comment

mgorny commented Apr 6, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024

mgorny commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

rjodinchr commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024

frasercrmck commented Apr 8, 2024

mgorny commented Apr 8, 2024