This is an archive of the discontinued LLVM Phabricator instance.

Remove support for enabling / disabling multi-threading at runtime.
ClosedPublic

Authored by zturner on Jun 9 2014, 3:47 PM.

Details

Summary

This patch removes the functions llvm_start_multithreaded() and llvm_stop_multithreaded(), and changes llvm_is_multithreaded() to return a constant value based on the value of the compile-time definition LLVM_ENABLE_THREADS.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 10250.Jun 9 2014, 3:47 PM
zturner retitled this revision from to Remove support for enabling / disabling multi-threading at runtime..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: dblaikie, chandlerc, rnk.
zturner added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.Jun 9 2014, 4:07 PM

Nice! I want to get a little more support for this direction before committing. Let me try to rope in some appropriate people. In the meantime, I had some comments.

include/llvm-c/Core.h
2856 ↗(On Diff #10250)

The C API is one of the few places where we have hard ABI stability guarantees. These should become no-op stubs.

include/llvm/Support/Threading.h
38 ↗(On Diff #10250)

nit: LLVM is in the 'star-on-the-right' camp.

lib/Support/Threading.cpp
28–32 ↗(On Diff #10250)

return bool(LLVM_ENABLE_THREADS) ?

zturner updated this revision to Diff 10252.Jun 9 2014, 4:27 PM
zturner edited edge metadata.

Address review comments.

reames added a subscriber: reames.EditedJun 9 2014, 5:36 PM

LGTM with documentation and asserts.

(Note: I don't have commit rights. You need approval from someone who does. And for this change, lots more feedback.)

include/llvm-c/Core.h
2852 ↗(On Diff #10252)

Which compile time flag? Where do I find information on this? (answer in comments)

include/llvm/Support/ManagedStatic.h
106 ↗(On Diff #10252)

This change looks questionable. Less because of your code than what was there before. Is there an assumption that llvm_shutdown_obj is only used in multithreaded builds? If so, should we assert for this?

include/llvm/Support/Threading.h
38 ↗(On Diff #10252)

While I'm generally a fan of standardizing on locked regions and standard locks, that should be a separate change.

lib/IR/Core.cpp
2703 ↗(On Diff #10252)

This interface is easily abused by a client written to the old API. You should add an assert here that the build is in fact multithreaded. Also, a comment explaining the history would be helpful. With the assert, this is the place many clients are going to first encounter your change.

lib/Support/ErrorHandling.cpp
41 ↗(On Diff #10252)

The changes in this file seem useful and standalone. Could they be submitted separately? (Using existing locking primitives.)

zturner added inline comments.Jun 10 2014, 11:42 AM
include/llvm/Support/ManagedStatic.h
106 ↗(On Diff #10252)

I don't think there's any such assumption. Actually, this was dead code before, because I couldn't find any instances of anyone ever using this constructor. llvm_shutdown_obj was only ever default-constructed.

Either way, I think this was only ever intended to be a convenience. Using this constructor with false is equivalent ot using the default constructor, and using it with true is equivalent to using it with false but manually calling llvm_start_multithreaded(). So I don't think we need to do anything here.

lib/Support/ErrorHandling.cpp
41 ↗(On Diff #10252)

Yes and no. This effort stemmed from a larger effort whose original goal was in fact to replace all uses of sys::Mutex with std::mutex / std::recursive_mutex. During review, it came up that I should remove llvm_start_multithreaded / llvm_stop_multithreaded as a pre-requisite. However, removing that necessitates a change here as well, due to the fact that install_fatal_error_handler now needs to be thread-safe.

So I could just go ahead and change these in a separate patch first, but I think people wanted the multi-threading patch done first.

zturner updated this revision to Diff 10294.Jun 10 2014, 12:40 PM
zturner edited the test plan for this revision. (Show Details)

Addressed latest set of review comments.

rnk accepted this revision.Jun 10 2014, 3:47 PM
rnk edited edge metadata.

lgtm

The commit message should mention that this had no performance impact on the clang self-host.

lib/IR/Core.cpp
2709–2710 ↗(On Diff #10294)

The previous behavior was to return false, so we should preserve that without asserting.

This revision is now accepted and ready to land.Jun 10 2014, 3:47 PM
zturner closed this revision.Jun 10 2014, 4:09 PM
zturner updated this revision to Diff 10306.

Closed by commit rL210600 (authored by @zturner).

chandlerc edited edge metadata.Jun 16 2014, 2:23 AM

I'm still really not happy with the approach for the global lock. This is
completely different from the approach you and I discussed, on a separate
patch with a separate review thread. Please *do not* fork reviews in this
way. Please revert this and resume the code review on the original thread.

/ I'm sorry if you're frustrated by the time it is taking to get this in,

but when one person has expressed specific concerns, I think it is a
mistake to move forward without sync-ing back up with them.

I'm not really sure what's going on here, but I did not submit this, and
I'm not sure why it has me listed as the submitter. Maybe rnk@ did this
without knowing what we discussed offline.

I will revert this in a few hours and move forward with the changes we
discussed offline, then resubmit. Sorry!