In D53912#1281584, @rnk wrote:

This sounds like it would defeat what I'm assuming is the intended purpose of intrin0.h, which is to reduce compile time. intrin.h is kind of enormous, and the compile time problems are well-documented. We should investigate what's up with intrin0.h and implement as many builtins as we need to support it.

CMAKE_LINKER MATCHES "lld-link\.exe"

@aganea, can you please elaborate on the need for the backslash that you added?

Thanks for the review @ruiu.

Thanks for the review @ruiu.

Adding Ilya who is the original author of that file.

@peter.smith Thanks for the review.

Ping for review please.

Updated to check if VFP is enabled for armhf too.

In D47217#1165279, @peter.smith wrote:

Can you elaborate on why the armhf buildbot failed and why you have excluded armhf from the MATCHES expression above. A simple test of a source file containing

__VFP_FP__

clang --target=arm-linux-gnueabihf -E vfp.c gives me

# 1 "vfp.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 361 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "vfp.c" 2
1

Unless I'm missing something a target of arm-linux-gnueabihf would define the macro unless the fpu were disabled with -mfpu=none so it should come through the test with the right answer.

In D49397#1166067, @phosek wrote:

What about C++ flags? libc++ is a C++ library so those are likely going to be important as well. Rather than lumping both C and C++ flags together into a single variable, we should probably consider adding two new keyword arguments to add_custom_libcxx, CMAKE_C_FLAGS and CMAKE_CXX_FLAGS, and use those to pass C and C++ flags separately to libc++ build.

I had to revert this last time as it was causing the armhf buildbot to fail. This should take care of that.

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.