[PATCH] D72301: [ORC] Fix the move-captured std::unique_ptr vs. std::function dilemma

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz created this revision.
sgraenitz added reviewers: lhames, bkramer, pree-jackie, shafik.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

C++14 is there for a while now and the FIXME is getting confusing. There was an earlier attempt to fix this the way (I think) it was indended originally: https://github.com/llvm/llvm-project/commit/ce74c3b19f5b#diff-f27b9f96303108227f0119d85d6c3ddf (It was rolled back here: https://github.com/llvm/llvm-project/commit/6baaa4be7831#diff-f27b9f96303108227f0119d85d6c3ddf )

    ES->setDispatchMaterialization(
        [this](JITDylib &JD, std::unique_ptr<MaterializationUnit> MU) {
  +       auto Work = [MU = std::move(MU), &JD] { MU->doMaterialize(JD); };
  -       // FIXME: Switch to move capture once we have c++14.
  -       auto SharedMU = std::shared_ptr<MaterializationUnit>(std::move(MU));
  -       auto Work = [SharedMU, &JD]() { SharedMU->doMaterialize(JD); };
          CompileThreads->async(std::move(Work));
        });

Unfortunately this doesn't work, because the lambda that captures the std::unique_ptr is being passed to ThreadPool::async() as a std::function: Holding the std::unique_ptr its copy constructor is deleted implicitly, but the standard requires the value of std::function to be copy-constructible.

What's the options? We certainly want to use ThreadPool, so we are bound to std::function. I think we don't want to go through a raw pointer capture by-value here, especially seeing that we pass a std::function next..

The current workaround creates and copies a std::shared_pointer. That's quite expensive. In a first step we can move-capture it in the Work lambda to avoid the copy. I think we don't get around the creation (assuming we keep a smart pointer), so why not receive it as a std::shared_ptr directly? The only disproportionate overhead I see is in the materializeOnCurrentThread case. I changed that to not calling the function at all.

What do you think?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72301

Files:
  llvm/examples/SpeculativeJIT/SpeculativeJIT.cpp
  llvm/include/llvm/ExecutionEngine/Orc/Core.h
  llvm/lib/ExecutionEngine/Orc/LLJIT.cpp
  llvm/unittests/ExecutionEngine/Orc/CoreAPIsTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72301.236446.patch
Type: text/x-patch
Size: 5082 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200106/a08ceb63/attachment.bin>