[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

thakis added a comment. https://clang.llvm.org/docs/ThinLTO.html#controlling-backend-parallelism should probably be updated to mention "all". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75153/new/ https://reviews.llvm.org/D75153

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. In D75153#1987538 , @MaskRay wrote: > I remember I saw a related bugs.llvm.org report yesterday but I can't find it > now... This? https://bugs.llvm.org/show_bug.cgi?id=45556 Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

MaskRay added a comment. In D75153#1942336 , @aganea wrote: > In D75153#1906580 , @MaskRay wrote: > > > Does `taskset -c 0-3 lld -flavor ...` restrict the number of cores? > > > > cpu_set_t cpu; > >

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. In D75153#1987320 , @phosek wrote: > In D75153#1987272 , @phosek wrote: > > > We've started seeing `llvm-cov` on our Linux bots with this error: > > > > terminating with uncaught exception

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

phosek added a comment. In D75153#1987272 , @phosek wrote: > We've started seeing `llvm-cov` on our Linux bots with this error: > > terminating with uncaught exception of type std::__2::system_error: thread > constructor failed: Resource temporarily

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

phosek added a comment. Herald added a reviewer: MaskRay. We've started seeing `llvm-cov` on our Linux bots with this error: terminating with uncaught exception of type std::__2::system_error: thread constructor failed: Resource temporarily unavailable Specifically, we're running `llvm

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. In D75153#1947956 , @zero9178 wrote: > This patch seems to break when disabling threading (aka LLVM_ENABLE_THREADS > == 0) as get_threadpool_strategy is undefined therefore creating linker > failures in clang. (Tested on Windows)

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. In D75153#1947956 , @zero9178 wrote: > This patch seems to break when disabling threading (aka LLVM_ENABLE_THREADS > == 0) as get_threadpool_strategy is undefined therefore creating linker > failures in clang. (Tested on Windows)

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

zero9178 added a comment. This patch seems to break when disabling threading (aka LLVM_ENABLE_THREADS == 0) as get_threadpool_strategy is undefined therefore creating linker failures in clang. (Tested on Windows) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

MaskRay added inline comments. Comment at: lld/test/ELF/basic.s:252 -# RUN: not ld.lld %t --thinlto-jobs=0 2>&1 | FileCheck --check-prefix=NOTHREADSTHIN %s -# RUN: not ld.lld %t --plugin-opt=jobs=0 2>&1 | FileCheck --check-prefix=NOTHREADSTHIN %s -# NOTHREADSTHIN:

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

This revision was automatically updated to reflect the committed changes. aganea marked an inline comment as done. Closed by commit rG09158252f777: [ThinLTO] Allow usage of all hardware threads in the system (authored by aganea). Changed prior to commit:

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea marked 3 inline comments as done. aganea added a subscriber: respindola. aganea added a comment. In D75153#1906580 , @MaskRay wrote: > Does `taskset -c 0-3 lld -flavor ...` restrict the number of cores? > > cpu_set_t cpu; > sched_getaffinity(0,

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

abrachet added inline comments. Comment at: llvm/include/llvm/Support/Threading.h:201 + /// hardware core is used. + inline ThreadPoolStrategy heavyweight_hardware_concurrency(StringRef Num) { +Optional S = Nit: Remove `inline`

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

MaskRay added a comment. Does `taskset -c 0-3 lld -flavor ...` restrict the number of cores? cpu_set_t cpu; sched_getaffinity(0, sizeof(cpu), ); CPU_COUNT() CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75153/new/ https://reviews.llvm.org/D75153

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

rnk added a comment. I think making the build system responsible for running the backend actions is the ideal solution, actually. The main reason we have all this threading logic in the linker is to make it easy for users of traditional build systems to use ThinLTO with a few flag flips. We

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. Thanks Reid! I will leave this open for a few days, in case there are any other feedbacks. As for `-fthinlto-index`, we have someone looking at distributing it remotely with Fastbuild. I think it is reasonable on the short-term to let the build system handle that (I

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I'm busy and I haven't looked at the code in detail, but I'm OK with going back to the old way of doing things. I think honoring user requests to use more threads than cores is an important use

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea updated this revision to Diff 248254. aganea edited the summary of this revision. aganea edited reviewers, added: dexonsmith, ruiu; removed: RobRich999, espindola. aganea added subscribers: ruiu, RobRich999. aganea added a comment. Herald added a reviewer: espindola. Simplify. Revert to

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. @mehdi_amini Agreed. In that case, I just need to slightly change this function to ensure threads are equally dispatched on all CPU sockets regardless of the thread strategy.

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

mehdi_amini added a comment. > /opt:lldltojobs=N -- limit usage to N threads, but constrained by usage of > heavyweight_hardware_concurrency(). I really dislike this behavior: this seems user hostile to me. I would either: - honor the user request (eventually display a warning), this is in

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

RobRich999 added a comment. Based upon the description, I think this patch is more applicable than just targeting a specific AMD proc family since it allows the end-user a choice for maximizing threading with both CMT and SMT on all supported platforms. BTW, until if/when this patch lands, I

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea added a comment. If a simpler (more yucky?) patch is needed to fix what @thakis was suggesting in https://reviews.llvm.org/D71775#1891709, and if we don't want this extra new flag, we can also check the CPU brand for "AMD Opteron", and keep the old behavior in that case. Repository:

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

aganea created this revision. aganea added reviewers: tejohnson, thakis, rnk, RobRich999. Herald added subscribers: cfe-commits, dang, dexonsmith, mikhail.ramalho, steven_wu, MaskRay, aheejin, hiraditya, arichardson, inglorion, sbc100, emaste. Herald added a reviewer: espindola. Herald added