Skip to content

Conversation

@jackm97
Copy link

@jackm97 jackm97 commented Nov 26, 2024

Because libclc-remangler commands executed in cmake use libclc::libclc-remangler, builds where CMAKE_CROSSCOMPILING=ON do not work correctly since cmake assumes the target is not executable on the host machine.

This change modifies the custom commands to use the cache variable LIBCLC_REMANGLER_PATH instead, where this variable is set in a similar way as llvm tools such as llvm-config by calling compile_native_tool.

Because libclc-remangler commands executed in cmake use
`libclc::libclc-remangler`, builds where `CMAKE_CROSSCOMPILING=ON` do
not work correctly since cmake assumes the target is not executable on
the host machine.

This change modifies the custom commands to use the cache variable
`LIBCLC_REMANGLER_PATH` instead, where this variable is set in a similar
way as llvm tools such as `llvm-config` by calling
`compile_native_tool`.

Signed-off-by: Jack Myers <jackhyers97@gmail.com>
@jackm97 jackm97 marked this pull request as ready for review November 26, 2024 00:36
@jackm97 jackm97 requested a review from a team as a code owner November 26, 2024 00:36
@jackm97 jackm97 requested a review from jchlanda November 26, 2024 00:36
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @hvdijk who fixed cross-compilation issues upstream

if(CMAKE_CROSSCOMPILING)
if(LLVM_NATIVE_TOOL_DIR AND NOT LIBCLC_REMANGLER_PATH)
if(EXISTS "${LLVM_NATIVE_TOOL_DIR}/libclc-remangler${LLVM_HOST_EXECUTABLE_SUFFIX}")
set(LLVM_CONFIG_PATH "${LLVM_NATIVE_TOOL_DIR}/libclc-remangler${LLVM_HOST_EXECUTABLE_SUFFIX}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be setting LIBCLC_REMANGLER_PATH?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should've but I have decided to not do all the cross compiling logic manually, instead using setup_host_tool.

add_custom_command( OUTPUT "${builtins_remangle_path}"
COMMAND ${CMAKE_COMMAND} -E make_directory ${LIBCLC_LIBRARY_OUTPUT_INTDIR}
COMMAND libclc::libclc-remangler
COMMAND ${LIBCLC_REMANGLER_PATH}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the variable should be named libclc-remangler_exe like the other tools: clang_exe, llvm-link_exe?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing as my previous comment above, but yes:

Yes it should've but I have decided to not do all the cross compiling logic manually, instead using setup_host_tool.

add_dependencies(libclc-remangler NativeLibCLCRemangler)
endif()
else()
set(LIBCLC_REMANGLER_PATH "${CMAKE_CURRENT_BINARY_DIR}/bin/libclc-remangler" CACHE STRING "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure relying on the path like this is the best idea. Perhaps we can get the path to the binary using the libclc-remangler target itself? Does $<TARGET_PROPERTY:libclc-remangler,TARGET_FILE> work?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing again 😅:

Same thing as my previous comment above, but yes:

Yes it should've but I have decided to not do all the cross compiling logic manually, instead using setup_host_tool.


# If we've not already imported a libclc-remangler tool from an external build,
# set it up now.
if(NOT TARGET libclc::libclc-remangler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment makes me realise this PR breaks the ability to import libclc-remangler from another build, using LIBCLC_CUSTOM_LLVM_TOOLS_BINARY_DIR. We should keep that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is actually a bit confusing to me. I found PR #13743 that introduced this, but as far as I can tell, this breaks the LIBCLC_CUSTOM_LLVM_TOOLS_BINARY_DIR functionality since the target is never imported through anything like find_package.

I'm realizing now that this might be because I was testing everything with CMAKE_CROSSCOMPILING enabled which may have messed with the target import.

Regardless, the libclc::libclc-remangler pattern seems to be unique among the rest of LLVM where other tools used during the build that are also built as a part of LLVM use things like compile_native_tool or setup_host_tool + LLVM_NATIVE_TOOL_DIR which is assigned automatically if needed.

My latest commit uses libclc-remangler_target which is populated by setup_host_tool. But I think LIBCLC_CUSTOM_LLVM_TOOLS_BINARY_DIR might still be broken.

I see why LIBCLC_CUSTOM_LLVM_TOOLS_BINARY_DIR is used since it can be built as a standalone. I'm wondering if in the case that it has not been set, then it should be set to LLVM_NATIVE_TOOL_DIR if it has been set. Thoughts?

Jack Myers added 2 commits November 26, 2024 12:40
Because libclc-remangler commands executed in cmake use
`libclc::libclc-remangler`, builds where `CMAKE_CROSSCOMPILING=ON` do
not work correctly since cmake assumes the target is not executable on
the host machine.

This change modifies the custom commands to use the cache variable
`LIBCLC_REMANGLER_PATH` instead, where this variable is set in a similar
way as llvm tools such as `llvm-config` by calling
`compile_native_tool`.

Signed-off-by: Jack Myers <jackhyers97@gmail.com>
Because libclc-remangler commands executed in cmake use
`libclc::libclc-remangler`, builds where `CMAKE_CROSSCOMPILING=ON` do
not work correctly since cmake assumes the target is not executable on
the host machine.

This change modifies the custom commands to use the cache variable
`LIBCLC_REMANGLER_PATH` instead, where this variable is set in a similar
way as llvm tools such as `llvm-config` by calling
`compile_native_tool`.

Signed-off-by: Jack Myers <jackhyers97@gmail.com>
@frasercrmck
Copy link
Contributor

Hi @jackm97. I've been comparing this with #16244 which is fixing the same issues (and more besides). I notice that the libclc-remangler changes are very similar. Did you not encounter issues with llvm-spirv when cross-compiling? I'm wondering how to proceed with the two PRs - do you have a preference?

@jackm97
Copy link
Author

jackm97 commented Dec 4, 2024

@frasercrmck, yes I do run into issues with cross-compiling that this PR alone does not fix, but not llvm-spirv I don't think.

It might help to add that my scenario is a pseudo-cross-compiling workflow using conda build from x86 to x86. This mostly just to isolate what's causing things to fail without adding the complexity of a true cross build.

Some issues I've found:

  • there's a CMake variable in the compilation of .bc files along the lines of SYCL_GCC_TOOLCHAIN that sets the --gcc-toolchain flag. Problem is, this flag only works if clang can figure out the target triple. A better flag would likely be --gcc-install-dir as it explicitly specifies where to find it without heuristics derived from --target. It also seems --gcc-toolchain deprecation is at the very least being discussed, if not already the agreed upon direction
  • There's also no way to provide a sysroot for the build phases using the natively compiled sycl clang
  • even if the gcc-install-dir and sysroot is set, I've noticed weird issues with standard headers not being found. I only discovered this by building everything in a ubi docker container without standard headers. This means in a build env where all the standard headers are available, these system headers are likely being used during parts or all of these compilation phases, which I imagine would really mess things up in a true cross build
  • Similarly, if dependencies are not in the standard system dirs, compilation phases that launch the newly built clang are not found, despite passing in required flags to variables like CMAKE_CXX_FLAGS
  • There are multiple CMake files that use generators to specify the clang exe which doesn’t work with the default behavior that builds the native and target builds at the same time. There are likely other places as well that incorrectly use the build tools, not the native. These should all be converted to the setup_host_tool and get_host_tool_path

I've worked around the issue with flags needed for cross compiling by putting clang config files in the build/NATIVE/bin directory (e.g. -clang.cfg) with the flags required, after configuring with CMake. This was a quick and brute force fix that eliminated the need to alter the LLVM project itself, but I’m not sure if this makes a good permanent solution. That said, it’s worked really well so far.

IIRC, it's important to specify the triple implicitly via config file naming scheme, rather than by passing it via the target flag by cmd line (which I tried by modifying cmake files) or within the config files themselves. This makes sense, some clang calls use the target flag which overrides the target set in the config file. It seems clang still respects the triple encoded in the cfg filename wrt sysroot layout while still allowing for different targets to specified on the cmdline.

Ultimately it boils down to two issues: not all CMake files are adhering to LLVM patterns for native tools used during the build and flags necessary for cross compiling like sysroot and gcc install location not getting passed to the custom targets calling clang.

@jackm97
Copy link
Author

jackm97 commented Dec 4, 2024

I'm wondering how to proceed with the two PRs - do you have a preference?

I'll take a look at the PR in the morning, but it's a fairly simple change, so I don't think I'll have a preference other than that I was looking forward to my first contribution to LLVM 😅

In all seriousness, if it is pretty much the same PR, it's not a big deal. Whatever makes the most sense :)

@hvdijk
Copy link
Contributor

hvdijk commented Dec 4, 2024

Thanks @jackm97. We will be building in slightly different ways, please do let me know if you should find that my PR doesn't cover your use.

@jackm97
Copy link
Author

jackm97 commented Dec 24, 2024

Closing now that #16244 is merged

@jackm97 jackm97 closed this Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants