r257808 - Don't build jobs for the same Action + ToolChain twice.
Justin Lebar via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 15 17:35:56 PST 2016
If you revert this you'll have to revert three other dependent
patches. I'm looking at a fix, if you're able to sit tight for
fifteen minutes or so.
If not, I can at least contribute a testcase; that much is done.
On Fri, Jan 15, 2016 at 5:33 PM, Eric Christopher <echristo at gmail.com> wrote:
> I'm afk at the moment please revert and add a test case if you can?
>
>
> On Fri, Jan 15, 2016, 5:24 PM Chris Bieneman <beanz at apple.com> wrote:
>>
>> +Eric,
>>
>> If this patch can’t be quickly fixed, can it please be reverted?
>>
>> -Chris
>>
>> On Jan 15, 2016, at 4:36 PM, Chris Bieneman via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>>
>> This change breaks darwin fat binary support.
>>
>> > ./bin/clang++ ~/dev/scratch/hello_world.cpp -arch armv7 -arch armv7s
>> > -isysroot
>> > /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/
>> fatal error:
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo:
>> /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out and
>> /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out have
>> the same architectures (armv7) and can't be in the same fat output file
>> clang-3.9: error: lipo command failed with exit code 1 (use -v to see
>> invocation)
>>
>> Can someone more familiar with this code please take a look?
>>
>> Thanks,
>>
>> -Chris
>>
>> On Jan 14, 2016, at 1:41 PM, Justin Lebar via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>>
>> Author: jlebar
>> Date: Thu Jan 14 15:41:21 2016
>> New Revision: 257808
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=257808&view=rev
>> Log:
>> Don't build jobs for the same Action + ToolChain twice.
>>
>> Summary:
>> Right now if the Action graph is a DAG and we encounter an action twice,
>> we will run it twice.
>>
>> This patch is difficult to test as-is, but I have testcases for this as
>> used within CUDA compilation.
>>
>> Reviewers:
>>
>> Subscribers:
>>
>> Modified:
>> cfe/trunk/include/clang/Driver/Driver.h
>> cfe/trunk/lib/Driver/Driver.cpp
>>
>> Modified: cfe/trunk/include/clang/Driver/Driver.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Driver.h?rev=257808&r1=257807&r2=257808&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Driver/Driver.h (original)
>> +++ cfe/trunk/include/clang/Driver/Driver.h Thu Jan 14 15:41:21 2016
>> @@ -21,6 +21,7 @@
>> #include "llvm/Support/Path.h" // FIXME: Kill when CompilationInfo lands.
>>
>> #include <list>
>> +#include <map>
>> #include <memory>
>> #include <set>
>> #include <string>
>> @@ -381,10 +382,13 @@ public:
>>
>> /// BuildJobsForAction - Construct the jobs to perform for the
>> /// action \p A and return an InputInfo for the result of running \p A.
>> + /// Will only construct jobs for a given (Action, ToolChain) pair once.
>> InputInfo BuildJobsForAction(Compilation &C, const Action *A,
>> const ToolChain *TC, const char *BoundArch,
>> bool AtTopLevel, bool MultipleArchs,
>> - const char *LinkingOutput) const;
>> + const char *LinkingOutput,
>> + std::map<std::pair<const Action *,
>> std::string>,
>> + InputInfo> &CachedResults) const;
>>
>> /// Returns the default name for linked images (e.g., "a.out").
>> const char *getDefaultImageName() const;
>> @@ -441,6 +445,16 @@ private:
>> /// the driver mode.
>> std::pair<unsigned, unsigned> getIncludeExcludeOptionFlagMasks() const;
>>
>> + /// Helper used in BuildJobsForAction. Doesn't use the cache when
>> building
>> + /// jobs specifically for the given action, but will use the cache when
>> + /// building jobs for the Action's inputs.
>> + InputInfo BuildJobsForActionNoCache(
>> + Compilation &C, const Action *A, const ToolChain *TC,
>> + const char *BoundArch, bool AtTopLevel, bool MultipleArchs,
>> + const char *LinkingOutput,
>> + std::map<std::pair<const Action *, std::string>, InputInfo>
>> + &CachedResults) const;
>> +
>> public:
>> /// GetReleaseVersion - Parse (([0-9]+)(.([0-9]+)(.([0-9]+)?))?)? and
>> /// return the grouped values as integers. Numbers which are not
>>
>> Modified: cfe/trunk/lib/Driver/Driver.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=257808&r1=257807&r2=257808&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Driver/Driver.cpp (original)
>> +++ cfe/trunk/lib/Driver/Driver.cpp Thu Jan 14 15:41:21 2016
>> @@ -1632,6 +1632,8 @@ void Driver::BuildJobs(Compilation &C) c
>> if (A->getOption().matches(options::OPT_arch))
>> ArchNames.insert(A->getValue());
>>
>> + // Set of (Action, canonical ToolChain triple) pairs we've built jobs
>> for.
>> + std::map<std::pair<const Action *, std::string>, InputInfo>
>> CachedResults;
>> for (Action *A : C.getActions()) {
>> // If we are linking an image for multiple archs then the linker wants
>> // -arch_multiple and -final_output <final image name>. Unfortunately,
>> this
>> @@ -1651,7 +1653,7 @@ void Driver::BuildJobs(Compilation &C) c
>> /*BoundArch*/ nullptr,
>> /*AtTopLevel*/ true,
>> /*MultipleArchs*/ ArchNames.size() > 1,
>> - /*LinkingOutput*/ LinkingOutput);
>> + /*LinkingOutput*/ LinkingOutput, CachedResults);
>> }
>>
>> // If the user passed -Qunused-arguments or there were errors, don't
>> warn
>> @@ -1779,19 +1781,38 @@ static const Tool *selectToolForJob(Comp
>> return ToolForJob;
>> }
>>
>> -InputInfo Driver::BuildJobsForAction(Compilation &C, const Action *A,
>> - const ToolChain *TC, const char
>> *BoundArch,
>> - bool AtTopLevel, bool MultipleArchs,
>> - const char *LinkingOutput) const {
>> +InputInfo Driver::BuildJobsForAction(
>> + Compilation &C, const Action *A, const ToolChain *TC, const char
>> *BoundArch,
>> + bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,
>> + std::map<std::pair<const Action *, std::string>, InputInfo>
>> &CachedResults)
>> + const {
>> + std::pair<const Action *, std::string> ActionTC = {
>> + A, TC->getTriple().normalize()};
>> + auto CachedResult = CachedResults.find(ActionTC);
>> + if (CachedResult != CachedResults.end()) {
>> + return CachedResult->second;
>> + }
>> + InputInfo Result =
>> + BuildJobsForActionNoCache(C, A, TC, BoundArch, AtTopLevel,
>> MultipleArchs,
>> + LinkingOutput, CachedResults);
>> + CachedResults[ActionTC] = Result;
>> + return Result;
>> +}
>> +
>> +InputInfo Driver::BuildJobsForActionNoCache(
>> + Compilation &C, const Action *A, const ToolChain *TC, const char
>> *BoundArch,
>> + bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,
>> + std::map<std::pair<const Action *, std::string>, InputInfo>
>> &CachedResults)
>> + const {
>> llvm::PrettyStackTraceString CrashInfo("Building compilation jobs");
>>
>> InputInfoList CudaDeviceInputInfos;
>> if (const CudaHostAction *CHA = dyn_cast<CudaHostAction>(A)) {
>> // Append outputs of device jobs to the input list.
>> for (const Action *DA : CHA->getDeviceActions()) {
>> - CudaDeviceInputInfos.push_back(
>> - BuildJobsForAction(C, DA, TC, nullptr, AtTopLevel,
>> - /*MultipleArchs*/ false, LinkingOutput));
>> + CudaDeviceInputInfos.push_back(BuildJobsForAction(
>> + C, DA, TC, nullptr, AtTopLevel,
>> + /*MultipleArchs*/ false, LinkingOutput, CachedResults));
>> }
>> // Override current action with a real host compile action and
>> continue
>> // processing it.
>> @@ -1822,7 +1843,7 @@ InputInfo Driver::BuildJobsForAction(Com
>> TC = &C.getDefaultToolChain();
>>
>> return BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel,
>> - MultipleArchs, LinkingOutput);
>> + MultipleArchs, LinkingOutput,
>> CachedResults);
>> }
>>
>> if (const CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) {
>> @@ -1831,7 +1852,8 @@ InputInfo Driver::BuildJobsForAction(Com
>> assert(CDA->getGpuArchName() && "No GPU name in device action.");
>> return BuildJobsForAction(C, *CDA->begin(),
>> C.getCudaDeviceToolChain(),
>> CDA->getGpuArchName(), CDA->isAtTopLevel(),
>> - /*MultipleArchs*/ true, LinkingOutput);
>> + /*MultipleArchs*/ true, LinkingOutput,
>> + CachedResults);
>> }
>>
>> const ActionList *Inputs = &A->getInputs();
>> @@ -1847,9 +1869,9 @@ InputInfo Driver::BuildJobsForAction(Com
>> // need to build jobs for device-side inputs it may have held.
>> if (CollapsedCHA) {
>> for (const Action *DA : CollapsedCHA->getDeviceActions()) {
>> - CudaDeviceInputInfos.push_back(
>> - BuildJobsForAction(C, DA, TC, "", AtTopLevel,
>> - /*MultipleArchs*/ false, LinkingOutput));
>> + CudaDeviceInputInfos.push_back(BuildJobsForAction(
>> + C, DA, TC, "", AtTopLevel,
>> + /*MultipleArchs*/ false, LinkingOutput, CachedResults));
>> }
>> }
>>
>> @@ -1863,7 +1885,7 @@ InputInfo Driver::BuildJobsForAction(Com
>> AtTopLevel && (isa<DsymutilJobAction>(A) ||
>> isa<VerifyJobAction>(A));
>> InputInfos.push_back(BuildJobsForAction(C, Input, TC, BoundArch,
>> SubJobAtTopLevel,
>> MultipleArchs,
>> - LinkingOutput));
>> + LinkingOutput,
>> CachedResults));
>> }
>>
>> // Always use the first input as the base input.
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>
More information about the cfe-commits
mailing list