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:50:31 PST 2016
http://reviews.llvm.org/D16250
On Fri, Jan 15, 2016 at 5:35 PM, Justin Lebar <jlebar at google.com> wrote:
> 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