[PATCH] D15390: [ThinLTO] Launch importing backends in parallel threads from gold plugin
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 22 15:33:29 PST 2015
tejohnson added a comment.
Per IRC discussion, will do some refactoring of splitCodeGen next, then subsequently rebase this patch on top of that. But I wanted to reply to the latest comments here and upload a new patch that addresses them first.
Comment at: tools/gold/gold-plugin.cpp:891
@@ +890,3 @@
+ // Run backend threads.
+ splitCodeGen(std::move(M), OSPtrs.vec(), options::mcpu, Features.getString(),
+ Options, RelocationModel, CodeModel::Default, CGOptLevel);
> splitCodeGen can take a ArrayRef too. Why do you need the vec?
Ah ok, fixed.
Comment at: tools/gold/gold-plugin.cpp:975
@@ +974,3 @@
+ // Close the output file descriptor before we pass it to gold.
> It seems odd how much work the destructor of TaskInfo is doing.
> Most of the work is here because gold is not thread safe, correct? It so, it seems better to write this code explicitly after
Changed TaskInfo::~TaskInfo into TaskInfo::cleanup and invoked explicitly on each task after the wait().
Comment at: tools/gold/gold-plugin.cpp:998
@@ +997,3 @@
+ std::vector<claimed_file *> Worklist;
+ for (claimed_file &F : Modules)
> Why do you need a worklist?
> Can't you just just a simple loop over Modules?
I think this was leftover from my original pre-ThreadPool implementation. Good point that it isn't needed. Updated to iterate over Modules as suggested.
Comment at: tools/gold/gold-plugin.cpp:1033
@@ +1032,3 @@
+ std::move(InputFile), std::move(OS), NewFilename.c_str(), TempOutFile);
> This can be just
> Tasks.emplace_back(new TaskInfo(std::move(InputFile), std::move(OS),
> NewFilename.c_str(), TempOutFile));
More information about the llvm-commits