[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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::opt`s 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Steven Noonan via Phabricator via cfe-commits]

tycho added a comment.

I did some local work to make this build and pass almost all tests on Linux as 
well, not to make use of the multi-process features but just to avoid having a 
separate "for Windows only" branch, and to ensure tests pass across platforms. 
Unfortunately my change is not at all acceptable for inclusion because it's 
extremely Linux-specific, and Unix/Program.inc very explicitly says "only use 
generic UNIX code here". I use `pidfd`s and a `timerfd` combined with 
`epoll_wait` to implement `sys::WaitMany`. It works well, but I'd be concerned 
about how to implement this as smoothly for any other *nix platform. 
pidfd/timerfd/epoll made it downright easy, but I started off trying to look at 
options that would work more universally on e.g. Linux *and* the BSDs, but I 
couldn't find anything that wouldn't require building a Rube-Goldberg machine 
juggling timers, alarms, signals, etc, etc.

One thing I noticed in this was one test in particular: `Clang :: 
Driver/cl-fallback.c`. It seems to expose a small design oversight. Since we 
don't ever use `Command::Execute` or the overrides for it, we don't ever fall 
back to the other command in `FallbackCommand::Execute`.

Another thing I see as a big problem for this patch is the change in return 
value for `sys::ExecuteAndWait`. I think the rationale of using `bool` would be 
a good one in a new project: separating execution failure from child process 
runtime failure makes sense. But changing it now does cause problems if you 
merge patches that expect the old return values, and the build silently 
continues (i.e. `if (llvm::sys::ExecuteAndWait(...))` is a common construct). I 
noticed this in particular when I rebased on top of the LLVM master branch, as 
there were several new `sys::ExecuteAndWait` callers (e.g. 
`llvm/tools/llvm-reduce`). The difference in return type would probably be less 
dramatic if it didn't invert the logic (i.e. previously 0 == success, but now 
`true` means success and `0 != true`, so the whole `if(ExecuteAndWait` thing 
breaks terribly). It'd actually be better if the function was renamed or took a 
different set of arguments, just to cause the build to explicitly break so the 
callers could be addressed.

Anyway, I'd love to see this patch or a successor to it move forward, as we'd 
like to start building things with LLVM on *all* platforms where I work, and 
this would greatly help with the issues for the Visual Studio users (which are 
the majority of the prospective users at my office).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Steven Noonan via Phabricator via cfe-commits]

tycho added a comment.

OK, those two problems were actually easy enough to fix.

  diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
  index 2f98cf9e04..93e974842f 100644
  --- a/lib/Driver/Driver.cpp
  +++ b/lib/Driver/Driver.cpp
  @@ -3241,8 +3241,8 @@ void Driver::BuildActions(Compilation &C, 
DerivedArgList &Args,
 }
 if (FinalPhase != phases::Preprocess) {
   if (Arg *A = Args.getLastArg(options::OPT__SLASH_MP)) {
  +  if (!StringRef(A->getValue()).getAsInteger(10, C.CoresToUse))
   C.CoresToUse = llvm::hardware_concurrency();
  -StringRef(A->getValue()).getAsInteger(10, C.CoresToUse);
   }
 }

And:

  diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
  index 93e974842f..c1e8e798d4 100644
  --- a/lib/Driver/Driver.cpp
  +++ b/lib/Driver/Driver.cpp
  @@ -3239,7 +3239,7 @@ void Driver::BuildActions(Compilation &C, 
DerivedArgList &Args,
   Args.eraseArg(options::OPT__SLASH_Yu);
   YcArg = YuArg = nullptr;
 }
  -  if (FinalPhase != phases::Preprocess) {
  +  if (!YcArg && FinalPhase != phases::Preprocess) {
   if (Arg *A = Args.getLastArg(options::OPT__SLASH_MP)) {
 if (!StringRef(A->getValue()).getAsInteger(10, C.CoresToUse))
   C.CoresToUse = llvm::hardware_concurrency();

The result is a thing of beauty:

F9989375: Taskmgr_GE0Zo74CYl.png 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Steven Noonan via Phabricator via cfe-commits]

tycho added a comment.

I rebased this myself on the release_90 branch and I was pleasantly surprised 
that I got the merge right on the first try (?!), and it actually works well 
without any significant changes (other than merge conflict resolutions).

I've run into two problems with it though:

- Running `clang-cl.exe /MP` without a number following the argument doesn't 
seem to automatically pick up the number of available available CPUs. I haven't 
debugged that yet, but it's probably a trivial fix.

- Precompiled headers aren't being built properly when using `clang-cl.exe /MP` 
under Visual Studio. If I do a clean build with e.g. `/MP16`, it errors 
immediately with this:

> 1>project_pch.cpp
>  1>CL : error : unable to read PCH file .\Release\.\/project_pch.pch: 'no 
> such file or directory'
>  1>CL : fatal error : PCH file '.\Release\.\/project_pch.pch' not found: 
> module file not found
>  1>Done building project "project.vcxproj" -- FAILED.

When running with `-v` on `clang-cl.exe`'s command line, I can see that it 
starts doing a compile with `-emit-pch` and another with `-emit-obj`, 
apparently running simultaneously. It looks like the object file needs the PCH 
to be compiled first, so it fails when it doesn't find the .pch file the other 
process should be working on. I bumped `msbuild.exe`'s verbosity up to see what 
it tried to do, and it only invoked clang-cl once, to compile one file 
(`project_pch.cpp`) with the "emit precompiled header" arguments on the command 
line. So this shouldn't be too crazy-tough to fix.

(I think my short-term/wrong workaround for this will be to ignore `/MP` when 
an input file is type `c++-header`. The more accurate solution would be to make 
the `.obj` depend on the `.pch`)

---

Has there been any further work on this change outside of what's recorded in 
this thread?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Angus Hewlett via Phabricator via cfe-commits]

angushewlett added a comment.

In D52193#1589657 , @aganea wrote:

>




> In D52193#1589150 , @angushewlett 
> wrote:
> 
>> They're issued in the right order, but the second doesn't wait for the first 
>> to complete.
> 
> 
> Ok, I thought the wait was handled by MSBuild. If I understand correctly what 
> you're saying, MSBuild issues both commands at the same time, but the second 
> `cl.exe /Yu /MP` somehow waits for the first `cl.exe /Yc` to complete? (maybe 
> by checking if the PCH is still open for write?)

That appears to be the case, yes.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

In D52193#1589096 , @yurybura wrote:

> Do you have any plans to make this PR compatible with trunk? Now MSVC with 
> /MP builds much faster than clang-cl (at least 2x faster for our project)...


I'll get back to this after the vacation (somewhere in August)

In D52193#1589150 , @angushewlett 
wrote:

> They're issued in the right order, but the second doesn't wait for the first 
> to complete.


Ok, I thought the wait was handled by MSBuild. If I understand correctly what 
you're saying, MSBuild issues both commands at the same time, but the second 
`cl.exe /Yu /MP` somehow waits for the first `cl.exe /Yc` to complete? (maybe 
by checking if the PCH is still open for write?)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Angus Hewlett via Phabricator via cfe-commits]

angushewlett added a comment.

In D52193#1457612 , @aganea wrote:

> That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild 
> should issue two different calls to clang-cl, one with /Yc and one /Yu /MP. 
> Can you check with ProcMon that commands are issued in the right order?


They're issued in the right order, but the second doesn't wait for the first to 
complete.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Yury Bura via Phabricator via cfe-commits]

yurybura added a comment.

Dear @aganea,
Do you have any plans to make this PR compatible with trunk? Now MSVC with /MP 
builds much faster than clang-cl (at least 2x faster for our project)... 
Thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Angus Hewlett via cfe-commits]

Even if I just issue one file with /Yc and none with /Yu, it still fails.
Looks like it possibly generates two processes, one to generate the pch and
the other to compile and link. A temp file is generated but no final pch;
an error is emitted "Unable to read pch file foo.pch".

On Sun, 7 Apr 2019, 14:19 Alexandre Ganea via Phabricator, <
revi...@reviews.llvm.org> wrote:

> aganea added a comment.
>
> That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild
> should issue two different calls to clang-cl, one with /Yc and one /Yu /MP.
> Can you check with ProcMon that commands are issued in the right order?
>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D52193/new/
>
> https://reviews.llvm.org/D52193
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

That is strange. As you can’t mix /Yc and /Yu on the command line, MSBuild 
should issue two different calls to clang-cl, one with /Yc and one /Yu /MP. Can 
you check with ProcMon that commands are issued in the right order?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support -- NOT FOR SUBMIT

[Angus Hewlett via Phabricator via cfe-commits]

angushewlett added a comment.

Having applied this patch to the clang latest (which took a bit of doing, and 
it's possible I messed up along the way) - all working in general, and a 
roughly 6x speed-up on my i7-7820X. However, PCH support does not seem to work 
correctly.

Specifically, it looks like when a file is set to build with /Yc (Create PCH), 
this is not executed serially with respect to the other jobs. Therefore it 
attempts to compile targets with /Yu (Use PCH) before the PCH has been 
generated, and fails.

When one or more files are passed with /Yc, PCH generation needs to execute to 
completion before the other jobs are started.

If I compile without using PCHs, it all works correctly (but clearly there's 
some performance hit).

Can anyone confirm if they've got /Yc & /Yu working correctly on their system?

Thanks,

  Angus.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52193/new/

https://reviews.llvm.org/D52193



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits