In D47224#1112505, @Hahnfeld wrote:

I'm not sure about this patch: These commands do not install the aliases, they are created in the build directory and may be needed for testing.

@peter.smith Thanks for the review.

In D44153#1031969, @asb wrote:

Hi Azharuddin, many thanks for the contribution. I hadn't spotted this part of LLVM's CMake build system, and it's definitely useful to be able to build just the RISCV backend. I've tried this locally, and the problem I'm seeing is that a good number of Clang and LLVM tests fail when you don't build X86 support. On my system, 351 unexpected failures across Clang+LLVM. This is a problem with the tests rather than this patch (they should really be disabled if there is no X86 support), but it does limit the usefulness of just building _only_ the RISC-V backend. Is it expected that you build an LLVM with X86 and RISCV, ensure 100% tests pass, then build with just the RISCV backend and ignore any new failures?

The problems I'm seeing aren't introduced by this patch, so LGTM. I'd be curious on how/if you work around the tests issue though.

In D44153#1031976, @asb wrote:

Actually, looking again I'm not having any problems building just the RISCV backend even without this patch. e.g. an invocation like the following seems fine:

cmake -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
  -DBUILD_SHARED_LIBS=True -DLLVM_USE_SPLIT_DWARF=True \
  -DLLVM_OPTIMIZED_TABLEGEN=True \
  -DLLVM_BUILD_TESTS=True \
  -DLLVM_TARGETS_TO_BUILD="RISCV" \
  -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="RISCV" ../
cmake --build .

Could you please clarify exactly which build problems this fixes?

Thanks for the review @smeenai .

In D41214#966392, @smeenai wrote:

The two should be equivalent unless CMAKE_SYSTEM_VERSION expands out to a value that's a variable name (which is unlikely, since it should be expanding out to a number), but this is cleaner and more consistent.

In D30274#692397, @rengolin wrote:

Hi, right, so Tim has approved it on the mailing list, which didn't get transferred here.

But he also said an additional interesting point why you have to use mcpu and not another mattr: this is a code path that only triggers with A53 because of its scheduler.

LGTM, too. Can you just add the scheduler info (instead of just saying it hits with A53) on the commit line?

thanks!

In D30274#692395, @rengolin wrote:

Can you update the context around using git diff -U999?

Updated the context around the change.

In D26289#588354, @beanz wrote:

I think there is actually a more general solution to this. At the top of the function we can just have:

if(CMAKE_INSTALL_RPATH)
  return()
endif()

Then we can remove the conditionals on CMAKE_INSTALL_RPATH, and just allow the target to inherit from the global setting, which is the default.

Chris, can you please commit this change. I do not have commit rights yet. Thanks.

In D25743#574916, @beanz wrote:

I don't understand what you're doing here, or the justification for it. The LLVM build system has no concept of a default sysroot, and I'm not sure I understand why you're creating one. Please explain what you're trying to do and why you feel that you need this.