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 <echri...@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 <be...@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@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@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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >> > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits