Hi! Since this is the last commit in Instructions.cpp, you can maybe help?

Ping! Any further comments?

Hi @tycho ! Sorry for not getting back earlier.
I implemented an alternate approach last year, which proved to be better (in terms of build times) than what I proposed in this demo patch. That is, using a thread pool instead of the process pool as implemented here. This makes compilation with /MP considerably faster, and allows for neat optimizations like file caching, re-using allocated memory pages, and possibly other things down the line. But before getting there, it requires somehow making cl::opt and the CommandLineParser thread-safe, thus revive this RFC thread. In essence, we want a mode where cl::opts have their data optionally stored in a stack context, so that we can safely call a CC1Command in each thread. I'll write a proposal to revive the RFC.

I guess CLANG_SPAWN_CC1 is/was mostly for debugging purposes. We have otherwise no use for that env var in production, other than for occasional testing.

I'm glad you're fixing this, this problem has came up in our profile traces as well.

Fixed NetBSD as suggested in rG88b8cb7215d4333ab990c99f21c7f92262ef02ef

Ping @evgeny777 @steven_wu!
Do you think you can take a look at this please? It'd be nice if it could land before the 10.0 branch. Thanks!

Attention Apple maintainers: after this patch, do not forget to add -DCLANG_SPAWN_CC1=ON to your release scripts, to retain your previous crash reporting behavior (see discussion above). @dexonsmith @arphaman

Thanks for the fix Markus! I can commit on your behalf.

Hi,
This patch fails to build with MSVC (here building with latest Visual Studio 2019 16.4.2).
Could you possibly take a look please? Thanks!

[801/3515] Building CXX object tools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\ConvertVectorToLLVM.cpp.obj
FAILED: tools/mlir/lib/Conversion/VectorToLLVM/CMakeFiles/MLIRVectorToLLVM.dir/ConvertVectorToLLVM.cpp.obj
C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1424~1.283\bin\HostX64\x64\cl.EXE  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\mlir\lib\Conversion\VectorToLLVM -ID:\llvm-project\mlir\lib\Conversion\VectorToLLVM -Iinclude -ID:\llvm-project\llvm\include -ID:\llvm-project\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zi /Zc:strictStrings /Oi /Zc:rvalueCast /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4345 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /MT /O2 /Ob2   -UNDEBUG  /EHs-c- /GR- -std:c++14 /showIncludes /Fotools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\ConvertVectorToLLVM.cpp.obj /Fdtools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\MLIRVectorToLLVM.pdb /FS -c D:\llvm-project\mlir\lib\Conversion\VectorToLLVM\ConvertVectorToLLVM.cpp
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): note: No constructor could take the source type, or constructor overload resolution was ambiguous
D:\llvm-project\mlir\lib\Conversion\VectorToLLVM\ConvertVectorToLLVM.cpp(117): note: see reference to function template instantiation 'llvm::iterator_range<mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>> mlir::ArrayAttr::getAsRange<mlir::IntegerAttr>(void)' being compiled
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): note: No constructor could take the source type, or constructor overload resolution was ambiguous
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): error C2672: 'llvm::make_range': no matching overloaded function found
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): error C2780: 'llvm::iterator_range<IteratorT> llvm::make_range(T,T)': expects 2 arguments - 1 provided
D:\llvm-project\llvm\include\llvm/ADT/iterator_range.h(54): note: see declaration of 'llvm::make_range'
[874/2733] Building CXX object lib\Target\AArch64\CMakeFiles\LLVMAArch64CodeGen.dir\AArch64ISelDAGToDAG.cpp.obj
ninja: build stopped: subcommand failed.

Thanks for taking the time @hans!

Updated as suggested by @rnk.
I've also removed ThreadPoolStrategy::PinThreads because it wasn't conclusive. In the best case, pinning threads to a core / hyperthread, was similar in performance to that of using a full CPU socket affinity; in the worst case, it was degrading performance. The NT scheduler seems to be doing a pretty good job here, so we'll stick with it :-)

Dropping this, I'll just deal with the existing process launching API.

In D71786#1812388, @riccibruno wrote:

Am I the only one who is a little bit uncomfortable with including a replacement to the system allocator in LLVM? It seems to me that this is something that is clearly under the responsibility of the C library. Now this might still be worth doing given the large performance improvement, but yuck...

• Integrated changes as suggested by @russell.gallop - Could you please re-apply the patch on a clean git checkout see if this works?
• Fixed usage of LLVM_RPMALLOC_PAGESIZE -- it seems attribute((unused)) doesn't always prevent the symbol from being evicted by LLD -- maybe using TLS callbacks is an edge case. I extended the usage of /INCLUDE to Clang, like for MSVC.
• Fixed enabling large pages in rpmalloc, when they are provided in the 'config' struct. Previously, AdjustTokenPrivileges wasn't called in this case, and that is required for VirtualAlloc(MEM_LARGEPAGES, ...) to work.

Changes as suggested by @hans.

In D71786#1809819, @riccibruno wrote:

Is rpcmalloc using large pages by default if possible ? I have been toying with using large pages for some of the bump-pointer allocators in clang, namely the allocator of ASTContext and the allocator of Sema, and I am getting a ~5% improvement in parsing speed.

Not by default, but you can use set LLVM_RPMALLOC_PAGESIZE=2M or set LLVM_RPMALLOC_PAGESIZE=1G to enable large pages. However I realize that is broken (crashes), I'll investigate and get back.

I've used this patch since you've published it, on different contexts, things look good from my user perspective.

• Changes as suggested by @rnk
• Fixed linking remarks-shlib as reported by @russell.gallop
• Reverted back to default rpmalloc settings (no more unlimited mode), ie. ENABLE_UNLIMITED_CACHE is 0 now.
• Support rpmalloc's ENABLE_STATISTICS - when enabled, allocation statistics will be printed on process exit.

In D71975#1803542, @riccibruno wrote:

So I need to:

• Add a getHugePageSize similar to sys::Process::getPageSize which will do its best to find out about the huge page size(s) on the platform. This needs to be done for Windows too.

In D71786#1801275, @russell.gallop wrote:

It fails creating a library

Hi! This patch broke the polly tests (the bot seems down). I wrote a fix here: 92b68c1937cd065a2fc44d18c1099de7da19b356 - it was trivial enough, but please let me know if you don't agree with the fix.

In D71775#1793767, @mehdi_amini wrote:

Will it make sense to say "I don't want hyper-threads" ?

Not sure I remember correctly, but I believe one motivation behind avoiding "hyper-threads" and other virtual cores was that while they improve slightly the performance, they also increase the peak memory requirements: using heavyweight_hardware_concurrency() seemed like a good default tradeoff for most end-users.

Note in the bar charts how the 28-core system is faster, when compared to the 36-core. The 28-core does not suffer from the "processor group" issue fixed in D71775, the 28-cores/56-threads all fit in a single "processor group", so they are all available to the application; while the 36-cores/72-threads are projected to the application as two "processor groups", out of which only one is used by default. After D71775, the 36-core is evidently faster.

Thank @amccarth, fixed!

In D70568#1789527, @hans wrote:

I looked over this again, and also studied CrashRecoveryContext some more.

I don't really understand why this patch needs to modify the code for how the CRC is enabled and installed, etc.

I thought all we need for in-process-cc1 is to add the DumpStackAndCleanupOnFailure flag and behavior, nothing more.

Do you think this is possible, or am I missing something?

In D71040#1783934, @evgeny777 wrote:

Strange this assertion probably means, that there are two summaries with external linkage in VTableVI. I tried reproducer out of curiosity, but it worked fine for me (no assertion on stage2 with ninja clang).

I was able to successfully create an ThinLTO-built clang.exe with this patch. However when compiling with assertions on, I'm still getting this:

Assertion failed: P.VTableVI.getSummaryList().size() == 1 || llvm::count_if( P.VTableVI.getSummaryList(), [&](const std::unique_ptr<GlobalValueSummary> &Summary) { return GlobalValue::isExternalLinkage(Summary->linkage()); }) <= 1, file D:\llvm-project\llvm\lib\Transforms\IPO\WholeProgramDevirt.cpp, line 852

I simply built a 2-stage clang with the following cmd-line:

stage 1: cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_LIBXML2=OFF -DLLVM_ENABLE_PROJECTS="clang;lld;llvm" -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_CXX_COMPILER="C:/Program Files/LLVM/bin/clang-cl.exe" -DCMAKE_LINKER="C:/Program Files/LLVM/bin/lld-link.exe" -DLLVM_ENABLE_LLD=true -DLLVM_ENABLE_PDB=ON

Yes this was added in rG18627115f4d2db5dc73207e0b5312f52536be7dd and rGe08b59f81d950bd5c8b8528fcb3ac4230c7b736c. It looks like it was only there to validate that specific refactoring.
Should I remove these two files, or simply commit the previous diff? (ie. -rw-rw----)

Ensure the access-rights commands (setfacl and chmod) won't fail the tests if the user doesn't have the appropriate rights to change permissions.
Added llvm/test/Other/umask.ll because it was also failing.

Addressed all comments.
Added CLANG_SPAWN_CC1 cmake flag & environment variable to control whether we want a cc1 process or not. It is disabled by default. I left out the if (APPLE) part in cmake, as suggested by @rnk.

Addressed comments, and:

• Removed CrashRecoveryContext::HandleCrash() and #pragma clang handle_crash which wasn't covered by any test and most likely deprecated by #pragma clang crash
• Added refcounting to CrashRecoveryContext::Enable/Disable as well as lazy installation of the exception handlers in CrashRecoveryContext.
• Partially reverted rG4624e83ce7b124545b55e45ba13f2d900ed65654/llvm/lib/Support/Unix/Signals.inc to simplify checks for IntSigs (and to fix the issue mentioned here). @vsk

All tests pass on Windows/MSVC2019/Clang9/10 and Linux/Clang9/GCC9.1.

I did this in driver.cpp:

// Whether the cc1 tool should be called inside the current process, or forked
// to a new new process.
bool UseCC1ProcessFork = CLANG_FORK_CC1;

Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

In D69825#1768111, @arphaman wrote:

@aganea Please disable the new behavior for the Darwin platform. We rely on the fact that Clang -cc1 processes crash to report crashes using system's crash reporting infrastructure.

Just after I hit Submit last night, I found a better way, by disabling ACL for that folder.
Ping @tstellar @Meinersbur in case they have an opinion.
Please let me know which solution you think is better.

You're right indeed Russell, my bad, this gain is not as important as I initially claimed -- however there's a gain.

You probably already know this, but the behavior between MSVC & Clang is different as to the value of the constant (testcase for PR43265).
See: https://godbolt.org/z/y4uvgp
Clang gives 0, while MSVC gives 5. This incompatibility is a potential source of failure, one of our tests broke because of that.

Thanks for analyzing this!

In D70568#1760185, @hans wrote:

Do I understand correctly that the main point is to get a stack trace when CrashRecoveryContext::RunSafely() fails, instead of just returning an error?

In D69825#1760373, @russell.gallop wrote:

It looks like the git apply didn't work, but the script continued so this was a duff experiment, please ignore. I'll try to fix this and run it again.

Thanks for the feedback Russell!