I don't think anybody is running doxygen over the Swift source right now, so I'll just escape the @ sign. That being said, can we improve this diagnostic? "command does not have an argument" is confusing when a given command does have an argument, just a malformed one. What do you think about "command must precede a word"? At least that gives a hint that the argument is not a "word" according to doxygen.

From the Swift language source (http://github.com/apple/swift):

In D62445#1616218, @davezarzycki wrote:

Just FYI – Having LLVM_ENABLE_PLUGINS_default depend on LLVM_ENABLE_PIC is a hack that should go away someday. Doing so requires that plugin dependencies either always build PIC (and therefore ignore LLVM_ENABLE_PIC), or are linked into the executables that load them and the plugin knows how to find them in the executable. In the case of the latter and on Mach-O platforms, this requires passing -bundle_loader $PATH_TO_EXECUTABLE to the linker when creating the plugin (a.k.a. "bundle"), but on ELF platforms, I'm not sure what needs to be done.

Just FYI – Having LLVM_ENABLE_PLUGINS_default depend on LLVM_ENABLE_PIC is a hack that should go away someday. Doing so requires that plugin dependencies either always build PIC (and therefore ignore LLVM_ENABLE_PIC), or are linked into the executables that load them and the plugin knows how to find them in the executable. In the case of the latter and on Mach-O platforms, this requires passing -bundle_loader $PATH_TO_EXECUTABLE to the linker when creating the plugin (a.k.a. "bundle"), but on ELF platforms, I'm not sure what needs to be done.

Hello – This change seems to have exposed a bug in -Wdocumentation argument parsing. For example, this warns when it shouldn't(?):

I just did a quick test of this patch and now stage two testing works (Fedora 30, x86-64)

I can try -M (a.k.a. --print-map). Is there a simple failing test from the LLVM or clang test suite that would be enlightening to try it with?

Well, as it turns out, -DLLVM_LDFLAGS is even less effective than the original CMAKE build options.

As it turns out, stage 2 still fails on my workstation, but that's because -DCMAKE_EXE_LINKER_FLAGS=-Wl,-O0 -DCMAKE_SHARED_LINKER_FLAGS=-Wl,-O0 does NOT work as one expects due to LLVM's cmake/modules/AddLLVM.cmake setting -Wl,-O3 *after* CMake's CMAKE_EXE_LINKER_FLAGS or CMAKE_SHARED_LINKER_FLAGS. I'm trying a new two stage build with -DLLVM_LDFLAGS=-Wl,-O0.

I'm doing a three-stage build, but perhaps not exactly the same way. I'd be surprised if it passes.

Actually, the bug is narrower and doesn't require Swift (my build-and-test script placed the blame incorrectly). One just needs to build llvm+clang+lld after this change, then the following tests fail:

This change breaks a ton of clang tests, albeit in the Swift fork of clang. Was this expected? If not, can we revert this for now?

Sorry for the stale copy-and-paste buffer noise. This did NOT break non-PIC builds.

This broke non-PIC builds. Fixed: r362399

Fixed: r361399

This breaks non-PIC builds. Was this planned or expected? Can we revert this until a fix is found?

Can we cherry-pick this into a release branch? Skylake (and newer) CPUs are far from rare these days, especially on cloud hosting providers.

I've verified that this fixes my Skylake-SP (Xeon 8168) workstation. Thanks!

https://bugs.llvm.org/show_bug.cgi?id=41016

I've got a patch pending for Swift. That being said, the clang diagnostics leave something to be desired. If __attribute__((used)) on definitions is sloppy at best or wrong at worst, then that should have a warning/error. Want a bug report? Or does one exist already?

I can't get Swift top-of-tree to build with gcc (8.3.1 20190223 (Red Hat 8.3.1-2)), and I doubt that it is supported, but I could be wrong.

Can we please revert this code? One cannot use dynamic_cast on a char *. Did you mean reinterpret_cast?

This change breaks building/testing the compiler with CLANG_DEFAULT_LINKER set to lld. Was this intentional? What should people do if they want to use CLANG_DEFAULT_LINKER and run the test suite?

Hot fix: r352996

This change broke building Swift on my Linux box. The compiler confusingly complains about the *same* template being both explicitly specialized and implicitly instantiated:

/home/dave/s/u/swift/stdlib/public/runtime/ProtocolConformance.cpp:34:38: error: explicit specialization of 'dump' after instantiation
template <> void ProtocolDescriptor::dump() const {
                                     ^
/home/dave/s/u/swift/stdlib/public/runtime/ProtocolConformance.cpp:34:18: note: implicit instantiation first required here
template <> void ProtocolDescriptor::dump() const {
                 ^

Is this expected?

Turns out my setup was busted and I thought I was building/testing AArch64 when I wasn't. I think you need to add // REQUIRES: aarch64-registered-target to the file.

My mistake. Something else is going on. I assumed based on past precedent that this was what was going on. (I'm not sure how many people at ARM are verifying that their AArch64 commits work when the 32-bit target is disabled).

FYI – The following test fails if the ARM 32-bit target is NOT built alongside the 64-bit ARM target: MC/ELF/cfi-b-key-frame.s

By the way, LLVM itself will benefit nicely from this change. For example, the code gen for llvm::StringSwitch will improve dramatically.

This was instantly reverted. Is there a new Differential # I can subscribe to?

In D55263#1325390, @spatel wrote:

In D55263#1325283, @davezarzycki wrote:

In D55263#1320043, @spatel wrote:

I just looked over the codegen changes so far, but I want to add some more knowledgeable x86 hackers to have a look too. There are 2 concerns:

1. Are there any known uarch problems with overlapping loads?
2. Are there any known uarch problems with unaligned accesses (either scalar or SSE)?
   
   If there's any perf data (either nano-benchmarks or full apps) to support the changes, that would be nice to see. This reminds me of PR33329: https://bugs.llvm.org/show_bug.cgi?id=33329 (can we close that now?)

Let's not lose sight of the big picture here. If uarch problems exist, are they *worse* than the cost of calling memcmp()? In other words, is the likely register spills, function call overhead, and dynamic algorithm selection (given the constantness of the size parameter is lost) worth it?

No real argument from me - I just wanted to be sure that nobody knew of some pre-existing uarch disaster potential. The change should be a nice win for the general x86 case. I've just added some nits in the inline comments.

In D55263#1320043, @spatel wrote:

I just looked over the codegen changes so far, but I want to add some more knowledgeable x86 hackers to have a look too. There are 2 concerns:

1. Are there any known uarch problems with overlapping loads?
2. Are there any known uarch problems with unaligned accesses (either scalar or SSE)?
   
   If there's any perf data (either nano-benchmarks or full apps) to support the changes, that would be nice to see. This reminds me of PR33329: https://bugs.llvm.org/show_bug.cgi?id=33329 (can we close that now?)

This change broke the test suite when building in /tmp (tmpfs) on Linux:

r347862 of this change broke the ability to run the test suite with the ARM64 backend enabled BUT without the ARM backend enabled. Was this intentional? See below:

Tested and verified with -DCLANG_DEFAULT_LINKER=lld

A bisect revealed this change as the source of test failures on my Red Hat Fedora 28 Linux box. How was/is this tested?

Hi @rsmith – I've verified this patch fixes the original issue. Thanks for the quick fix!

Committed as r330353

Ping. Any review feedback would be appreciated. Thanks!

It wasn't my suggestion. @thakis wrote: "We probably should have a separate bool in clang and key this off that and make -ftime-report set both."

Hi @efriedma – Sorry about that. I haven't directly used svn in years thanks to the git svn bridge.

A revert in practice just means undoing the changes. For example, while both git and svn have revert subcommands, you can also just take the original diff/patch, pipe it into patch -R, and if there are no conflicts, commit the result.

FYI – This test fails on my machine (Release+Assert):

I don't think this change is correct. To the best of my knowledge, SKL does support SGX but SKX does not.

I think this change is incomplete. The following clang test fails on my SKX machine: Preprocessor/predefined-arch-macros.c

Great! I'll revisit this patch in a week and see if it is still needed. Hopefully the author of D43578 will make this patch unnecessary by then.

I haven't proven that r329684 is the source of the regression, but it is the only timer related change in the last 24 hours as far as I can tell.

This was committed.

Yup. I've already done a ninja check-all with llvm, lld, clang, and clang-tools-extras, and everything seems fine.

LLVM: r326109
clang: r326110

I think this final diff addresses all concerns, including some amount of move-only type compatibility (it compiles, but I'm not sure the experience is pleasurable).