This is an archive of the discontinued LLVM Phabricator instance.

Setting the LLVM_TARGET_TRIPLE macro based on the LLVM_DEFAULT_TARGET_TRIPLE
ClosedPublic

Authored by nicolerabjohn on Nov 28 2022, 2:02 PM.

Details

Summary

Due to https://reviews.llvm.org/D137870, LLVM_TARGET_TRIPLE is no longer defined on the runtime path into compiler-rt. This patch creates a common block of code to set LLVM_TARGET_TRIPLE equal to the default for both the llvm- and runtime- paths to compiler-rt.

Diff Detail

Event Timeline

nicolerabjohn created this revision.Nov 28 2022, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 2:02 PM
nicolerabjohn requested review of this revision.Nov 28 2022, 2:02 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptNov 28 2022, 2:02 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
This comment was removed by nicolerabjohn.

Removing comment to allow for more generic use.

mgorny added inline comments.Nov 28 2022, 9:40 PM
llvm/cmake/modules/SetTargetTriple.cmake
2

Stray empty line here ;-).

What's the issue you're trying to address? I just landed D137451, does it address your issue?

What's the issue you're trying to address? I just landed D137451, does it address your issue?

compiler-rt doesn't configure anymore for us on AIX, due to an empty variable expansion (which is derived from LLVM_TARGET_TRIPLE):

$cmake -DLLVM_ENABLE_RUNTIMES=compiler-rt  ../llvm-project/runtimes
...
CMake Error at ../llvm-project/compiler-rt/cmake/Modules/CompilerRTUtils.cmake:367 (string):
  string sub-command REPLACE requires at least four arguments.
Call Stack (most recent call first):
  ../llvm-project/compiler-rt/CMakeLists.txt:120 (construct_compiler_rt_default_triple)

where the offending block is:

    set(COMPILER_RT_DEFAULT_TARGET_TRIPLE ${LLVM_TARGET_TRIPLE} CACHE STRING
          "Default triple for which compiler-rt runtimes will be built.")
...
  string(REPLACE "-" ";" LLVM_TARGET_TRIPLE_LIST ${COMPILER_RT_DEFAULT_TARGET_TRIPLE})

Removed extra empty line.

nicolerabjohn marked an inline comment as done.Dec 9 2022, 12:25 PM

LGTM. Some paths in compiler-rt still require LLVM_TARGET_TRIPLE, so commoning the setup makes sense

daltenty accepted this revision.Dec 9 2022, 12:29 PM

Sorry, what is the semantic difference between LLVM_DEFAULT_TARGET_TRIPLE and LLVM_TARGET_TRIPLE?

This comment was removed by daltenty.

I believe @phosek documents the intended semantic here in https://reviews.llvm.org/D137451:

LLVM_DEFAULT_TARGET_TRIPLE is the default value that's used by tools like clang or llc when you don't explicitly specify --target or --triple. LLVM_TARGET_TRIPLE is the target triple we're building for. LLVM_DEFAULT_TARGET_TRIPLE is used as a the default value for LLVM_TARGET_TRIPLE but they're independent.

I believe @phosek documents the intended semantic here in https://reviews.llvm.org/D137451:

LLVM_DEFAULT_TARGET_TRIPLE is the default value that's used by tools like clang or llc when you don't explicitly specify --target or --triple. LLVM_TARGET_TRIPLE is the target triple we're building for. LLVM_DEFAULT_TARGET_TRIPLE is used as a the default value for LLVM_TARGET_TRIPLE but they're independent.

If I rephrase it this way, does it mean the same thing?
LLVM_DEFAULT_TARGET_TRIPLE is the default for --target or --triple-like options in the tools we are building.
LLVM_TARGET_TRIPLE describes the environment where we expect those tools to run (and could be passed to the compiler being used to build those tools).

The lit tests, which I'm dealing with a great deal these days, don't seem to look at those parameters the same way. Each projects's lit.site.cfg.py.in file sets config.target_triple = "@LLVM_TARGET_TRIPLE@" (except for a couple of outliers that don't set target_triple at all, and maybe the runtimes which hide their lit config files in unobvious places). Now, I'm willing to believe that the CMake configuration parameters have evolved without lit following along, but if we're trying to come to some conclusion about What It All Means, then we need to make sure that lit keeps up.

I have to say, I'm not really convinced that LLVM_TARGET_TRIPLE really means what's mentioned above. It's still about the target, not about the host. CMake conjures up an LLVM_HOST_TRIPLE which the lit config files can capture, and that's the running environment for the built tools.

The discussion about the meanings of these things should be taken to Discourse, not buried in a couple of patch reviews, in any case.

The discussion about the meanings of these things should be taken to Discourse, not buried in a couple of patch reviews, in any case.

100% agree with you. I think we can straighten things out there, since this is well out of scope of this review.

The matter of fixing the definitions of these macros and long term refactoring aside, we plan to proceed with this patch if there are no objections as it's fixing a build break introduce by D137870 which is still very much a present issue for us.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2022, 5:05 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.