Page MenuHomePhabricator

Remove support for runtime multithreading
Needs ReviewPublic

Authored by zturner on Jun 13 2014, 11:42 AM.

Details

Reviewers
dblaikie
rnk
Summary

This is a resubmit of r210600 (Differential Revision D4076). This probably only needs a quick glance since the previous change was already reviewed, however this change differs from the previous submission in 2 important ways:

  1. Conversions from sys::Mutex to std::[recursive_]mutex are removed, to reduce the scope of the changes. Those will be submitted later as a separate changelist.
  1. Making ErrorHandling.cpp thread-safe was handled as a separate changelist (see D4140)

Diff Detail

Event Timeline

zturner updated this revision to Diff 10399.Jun 13 2014, 11:42 AM
zturner retitled this revision from to Remove support for runtime multithreading.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: dblaikie, rnk.
zturner set the repository for this revision to rL LLVM.
zturner added a subscriber: Unknown Object (MLST).
chandlerc added inline comments.
lib/Support/ErrorHandling.cpp
70–80 ↗(On Diff #10399)

All of this feels like a totally separate thing from removing the runtime control of multithreading.

Can you try to split this up into more isolated changes? I feel like this is still a combined patch of several things that really should be submitted independently.

emaste added a subscriber: emaste.Jun 13 2014, 12:00 PM
emaste added inline comments.
lib/Support/ErrorHandling.cpp
74 ↗(On Diff #10399)

To be pedantic, all implementations exhibit undefined behaviour if a mutex destroyed while owned, although the nature of that behavior may be different.

zturner added inline comments.Jun 13 2014, 12:03 PM
lib/Support/ErrorHandling.cpp
70–80 ↗(On Diff #10399)

Fair enough. Although if I'm going to do it that way, the error handling functions have to be made thread-safe first, and then the removal of llvm_start / stop multithreading changes have to go in second. Which is fine, so I'll do that.

74 ↗(On Diff #10399)

I'm actually submitting this *before* converting to std::*mutex though. Our own mutex implementation doesn't exhibit undefined behavior. std::mutex implementations do though.

zturner updated this revision to Diff 10404.Jun 13 2014, 1:41 PM
zturner updated this object.

Changes to ErrorHandling.cpp uploaded as a separate change (D4140). That change should be submitted first.

One more thing to split out, and I think this will actually look good.

lib/Support/ManagedStatic.cpp
27

This (and the related change to the interface) is the other unrelated cleanup that I would like to see separated out from the change which has policy effect.

Consider, if we need to revert the runtime control of multithreading for a brief period because something breaks, we shouldn't also lose this cleanup change.

I tried this once and ran into some difficulty, because before this change,
the Mutex is a pointer which null is an expected value if
llvm_start_multithreaded has not been called. So the function would have
to return a pointer to the mutex. That would require changing MutexGuard
to take a pointer instead of a reference, which would generate a ton of
churn.

I'll have another stab at it and see if there's a way to do it in a way
that has very little churn and still contributes to this change as well.

zturner updated this revision to Diff 10412.Jun 13 2014, 5:33 PM

Removed code that had been split out into a different patch.