LGTM

LGTM

LGTM with a minor style nit.

LGTM, thanks

LGTM

Hi Sam, this patch seems to have caused a correctness regression in the telecomm-gsm and toast benchmarks when compiled for Arm. Logs at http://lab.llvm.org:8011/builders/clang-native-arm-lnt-perf/builds/170/. Would you mind looking into this?

LGTM

LGTM

LGTM

• Switch to using ArrayRef instead of null-terminated array

LGTM, thanks

Ping.

Ping.

• Add LTOPostLink metadata, instead of internalising vcall visibility at LTO time

Partial linking will indeed prevent dropping the virtual functions, but it should not prevent clearing the pointer to the virtual function in the vtable. The linker should then be able to drop the virtual function body as part of --gc-sections during the final link.

LGTM

• Take the example from my earlier message, give the "main executable" and "DSO" hidden visibility, build the "main executable" with LTO and the "DSO" without LTO, and statically link them both into the same executable. We run into the same problem where the Plugin1 vtable is potentially not referenced and thus misoptimised. Yes, this is a violation of the LTO visibility rules, but the example shows that we only detect it sometimes. I think that if we did want to detect cases where the LTO visibility rules are clearly being violated, the outcome should be to issue a diagnostic and not to silently proceed with optimizations disabled, since the violation might be masking other undetected issues. That really seems orthogonal to this patch, though.

LGTM, thanks

LGTM

LGTM

LGTM

• Rebase
• Don't emit llvm.assume when not necessary (we already weren't checking for it's presence in GlobalDCE)
• s/"public"/"default"/ in IR docs

Out of interest, why does the ABI allow functions which don't have SVE args/returns to clobber the P registers? For Z registers, we've got to be compatible with old code which only needed to save the bottom half of v8-v15, but there should be no existing code which uses P registers, so we could enforce a mixture of callee- and caller-saved P registers for all code. Existing code is already compliant with this, because it doesn't touch the P regs.

In that example, with everything having default ELF visibility, all of the vtables will get vcall_visibility public, which can't be optimised by VFE, and won't ever be relaxed to one of the tighter visibilities.

Would this also be profitable for Thumb2?

Ok, but please add a comment, probably in AArch64.td, explaining this, and that turning it on by default is only temporary.

Ping

Ping

This patch LGTM now, and +1 to the idea of documenting the attacks we do/don't expect to be able to defend against.

Ping.

LGTM

LGTM

https://reviews.llvm.org/rL366557

LGTM

Split into separate functions for safe/profitable.

LGTM

LGTM, thanks

LGTM, thanks

Two of the new tests are failing on the Arm and AArch64 buildbots - see for example http://lab.llvm.org:8011/builders/libcxx-libcxxabi-libunwind-armv7-linux/builds/946/steps/test.libcxx/logs/FAIL%3A%20libc%2B%2B%3A%3Adeduct.fail.cpp

LGTM, thanks.

All of the libcxx bots are currently failing with this compile error:

libcxx/src/exception.cpp:14:12: fatal error: 'cxxabi.h' file not found

Ping.

In case you haven't noticed it yet, the aarch64 bots are now failing with an undefined symbol __tsan::__interception::real_setjmp: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/6879/steps/ninja%20check%201/logs/stdio

What about the tests for large stack frames?

Actually, I think cross-unwinding with EHABI is already completely broken, because we are doing raw memory accesses (not using the AddressSpace), so this is fine.

This won't work for cross-unwinding between a big and little endian system, I think it would be better to load the whole word, and mask out the correct byte.

Thanks, that's fixed the link errors, but we are now seeing some failures in TSan tests (http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/6874, http://lab.llvm.org:8011/builders/clang-cmake-aarch64-lld/builds/6874/steps/ninja%20check%201/logs/FAIL%3A%20ThreadSanitizer-aarch64%3A%3A%20fiber_longjmp.cc). Could these be related to one of your patches?

LGTM with one minor nit.

We are still seeing liner errors in the AArch64 build bots (http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/7487/steps/ninja%20check%202/logs/stdio), are you looking into this? If there's not a quick, obvious fix, it would be better to revert the original commit to get the bots back to green.

There are a number of buildbot failures which look related to this, e.g.

• http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/6730/steps/ninja%20check%201/logs/FAIL%3A%20UBSan-AddressSanitizer-armhf%3A%3Adiv-zero.cpp
• http://lab.llvm.org:8011/builders/clang-cmake-armv7-full/builds/6730/steps/ninja%20check%201/logs/FAIL%3A%20UBSan-AddressSanitizer-armhf%3A%3Adiv-zero.cpp

I think this could do with some more tests covering different stack layouts. In particular, we should check that we do the correct thing for functions which use frame or base pointers, and when large stack frames cause the immediates in STG and ADDG instructions go out of range.

LGTM

LGTM with one nit.

Committed https://reviews.llvm.org/rL365014

• Add a PhysReg parameter to ignoreCSRForAllocationOrder
• Check that the register is a GPR in the ARM implementation. The other register classes have the callee-saved regs last, so this doesn't make any difference to the generated code, but might avoid surprising behaviour in the future.