Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[Support] Move LLD parallel algorithms to LLVM.

Authored by zturner on May 9 2017, 7:03 PM.

Diff Detail


Event Timeline

zturner created this revision.May 9 2017, 7:03 PM
chandlerc edited edge metadata.May 9 2017, 7:29 PM

Maybe I'm stuck with an old diff or something?

748 ↗(On Diff #98385)

I find the fact that for_each is now getting looked up via ADL on the llvm::parallel::par type ... disturbing.

51–53 ↗(On Diff #98385)

I thought all of this was being moved to be implementation details of the algorithms?

Also, all the code appears to still be LLD style?

zturner added inline comments.May 9 2017, 7:33 PM
748 ↗(On Diff #98385)

I can certainly qualify the function call, but I don't know of a way to disable ADL if that's what you're suggesting.

51–53 ↗(On Diff #98385)

The executor stuff was all moved to an anonymous namespace inside of TaskGroup.cpp. TaskGroup cannot enjoy the same level of hiding because it needs to be usable from within a header file. Namely, Parallel.h. I can, however, move it to a detail namespace inside of Parallel.h if you prefer that.

also: Yes rui pointed out the names in a previous patch and I forgot to update them. All of them have been fixed except for the few instances in this file.

chandlerc added inline comments.May 9 2017, 7:48 PM
748 ↗(On Diff #98385)

It's "easy": foo(a, b, c) uses ADL and (foo)(a, b, c) doesn't use ADL. ;]

I'm not suggesting using (for_each) though. That seems a bit crazy.

Writing llvm::foo will also disable ADL. ADL only fires for unqualified names. But then writing the code gets super ugly:

llvm::for_each(llvm::parallel::par, ...);

Yuck. I guess I mildly prefer that? The standard sure chose an API that ends up requiring a lot of characters to type.

What do you or Rui think? I don't have strong feelings about which way you go here, so would generally defer to your or Rui's judgement.

51–53 ↗(On Diff #98385)

I see, thanks.

I think a detail namespace is a touch cleaner. I'm happy for you to do that in this patch, same with any name cleanup.

zturner updated this revision to Diff 98392.May 9 2017, 8:34 PM
  • Changed naming conventions on the leftover LLD-style variables.
  • Deleted TaskGroup.h and TaskGroup.cpp and moved corresponding code to Parallel.h and Parallel.cpp inside of the detail namespace.
zturner added inline comments.May 9 2017, 8:37 PM
748 ↗(On Diff #98385)

It's worse than that, because it's going to be llvm::parallel::for_each(llvm::parallel::par, ...). We could nuke the intermediate parallel namespace and go with llvm::for_each(llvm::par, ...), or we could do using llvm::parallel at the top of a file. The namespace is sufficiently scoped that bringing it in shouldn't be too onerous.

Personally I'm fine with any approach, including the ADL-based approach. That said, I don't think there's anything we can do to the API itself to alleviate this, so perhaps we don't need to do anything. Just leave it up to the users.

zturner updated this revision to Diff 98491.May 10 2017, 10:56 AM

Rebased after renaming variables on the LLD side.

ruiu added inline comments.May 10 2017, 3:09 PM
748 ↗(On Diff #98385)

Because we have using namespace llvm at beginning of this file, you don't need to qualify it with llvm::, no?

LGTM on the LLVM side. Feel free to submit when you and Rui are happy with the usage pattern for this in LLD.

67 ↗(On Diff #98491)

closing namespace comment?

ruiu accepted this revision.May 10 2017, 4:33 PM

I'm happy as long as you remove unnecessary llvm:: specifier. LGTM.

This revision is now accepted and ready to land.May 10 2017, 4:33 PM
This revision was automatically updated to reflect the committed changes.