This is an archive of the discontinued LLVM Phabricator instance.

Remove `clang/runtime`
AbandonedPublic

Authored by Ericson2314 on Apr 1 2021, 11:22 AM.

Details

Reviewers
beanz
phosek
Summary

This patch removes clang/runtime, after COMPILER_RT_INSTALL_PATH is
removed in order to simplify things for D99484.

As @phosek said in https://reviews.llvm.org/D99484#2662806

I'm not sure if we still need COMPILER_RT_INSTALL_PATH. That
variable is only used by clang/runtime/CMakeLists.txt which
predates runtimes/CMakeLists.txt and was AFAIK only ever used by
Apple. I think we should consider removing
COMPILER_RT_INSTALL_PATH. We'll need to check if
clang/runtime/CMakeLists.txt is still being used or not.

It turns out COMPILER_RT_INSTALL_PATH isn't only used by that, but we
might (hopefully!) be able to remove it annyways. If so, clang/runtime
can still be removed, which is a simplification we might as well do if
@phosek is right that it is no longer in use.

Diff Detail

Event Timeline

Ericson2314 created this revision.Apr 1 2021, 11:22 AM
Ericson2314 requested review of this revision.Apr 1 2021, 11:22 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 1 2021, 11:22 AM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
Ericson2314 added inline comments.Apr 1 2021, 11:23 AM
compiler-rt/cmake/base-config-ix.cmake
93

I'm a bit confused we'd ever want too install libs directly in ${CMAKE_INSTALL_PREFIX}, but that is technically the existing behavior, with the default value for COMPILER_RT_INSTALL_PATH, so I kept it.

Also remove clang/runtime from a TODO comment elsewhere in the tree

I put runtimes in there as it's evidentially its spiritual successor.

Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2021, 11:28 AM

@beanz do you know if clang/runtime is still being used?

Remove add_subdirectory(runtime) for deleted dir

So unfortunately it seems the clang/runtime's use was triggered by LLVM_BUILD_EXTERNAL_COMPILER_RT, which is in use in a few places still? (And not just Apple ones.)

On the other hand, since this is doing an *external* CMake for compiler-rt, I fail to see why COMPILER_RT_INSTALL_PATH is needed; why not just pass a different CMAKE_INSTALL_PREFIX?

Remove stray COMPILER_RT_INSTALL_PATH

I am uncertain about is the redefining of CMAKE_INSTALL_PREIX in
compiler-rt/cmake/base-config-ix.cmake. This occurs under
LLVM_TREE_AVAILABLE. If this is just for separate compiler-rt builds
(separate invocation of CMake, that is), this is fine. But if on the other hand
this is for combination builds in a single CMake run, then this sort of
redefinition is no good as it will effect more than compiler-rt.

See also D99860. We may still want to remove clang/runtime, but I don't think it's necessary. I will turn this revision into one on top of that just for for removing clang/runtime.

Ericson2314 retitled this revision from Remove clang/runtime and `COMPILER_RT_INSTALL_PATH` to Remove `COMPILER_RT_INSTALL_PATH`.Apr 4 2021, 9:47 AM
Ericson2314 edited the summary of this revision. (Show Details)
Ericson2314 edited the summary of this revision. (Show Details)
Ericson2314 retitled this revision from Remove `COMPILER_RT_INSTALL_PATH` to Remove `clang/runtime`.Apr 4 2021, 10:38 AM

D101497 is more safe/conservative and gets the job done to prepare for`GnuInstallDirs`. Closing in favor of that.

Ericson2314 abandoned this revision.May 4 2021, 7:43 PM
clang/runtime/CMakeLists.txt